-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Run clang-format in CI #15524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run clang-format in CI #15524
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
csharp/ApiDocs/_exported_templates/default/styles/search-worker.js
Fixed
Show resolved
Hide resolved
e641046
to
7c7e6cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a generated file, can we exclude it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can exclude onnxruntime/core/flatbuffers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can exclude
onnxruntime/core/flatbuffers
?
There are some other non-generated files there which we can still include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.lintrunner.toml
Outdated
'**/*.m', | ||
'**/*.mm', | ||
'**/*.cs', | ||
'**/*.java', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there may be some existing conventions for Java and C#, and also for WinML code.
@Craigacp @yuslepukhin @fdwr Are you all fine with these formatting changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing for now because I excluded them from this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Java API has a code formatter called spotless that runs as part of the test suite, if you apply this one it'll get very confused and fail all the CI pipelines with Java in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info! I excluded java. Does the current change look good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though the Java binding includes some C code with the extension .c
. You could apply the formatter to that, we don't currently do any formatting on the C code for the Java binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinchuby Did you intend to also rewrap enums?
Back when I was playing around with a half dozen formatters, I found this online clang formatter very quick and easy to try out different settings: https://zed0.co.uk/clang-format-configurator/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rewrap was a side effect of clangformat seeing it has enough horizontal space to fit everything. I am going to consult with chatgpt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't figure it out with the configurator or with chatgpt. Do you have suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chatgpt helpfully created a fix script: https://github.com/microsoft/onnxruntime/pull/15606/files
@edgchen1 Looks like there are potential build errors (not sure if it is flaky ci or true errors). Should I instead start from c++ then expand to other languages in separate prs? I will revert the subsequent changes |
sure, that sounds good |
Now just
|
Description
Run clang-format in CI. Formatted all c/c++, objective-c/c++ files.
Excluded
because they contain assembly or is data heavy
Motivation and Context
Coding style consistency