Skip to content

Conversation

justinchuby
Copy link
Contributor

@justinchuby justinchuby commented Apr 15, 2023

Description

Run clang-format in CI. Formatted all c/c++, objective-c/c++ files.

Excluded

    'onnxruntime/core/mlas/**',
    'onnxruntime/contrib_ops/cuda/bert/tensorrt_fused_multihead_attention/**',

because they contain assembly or is data heavy

Motivation and Context

Coding style consistency

@justinchuby justinchuby requested a review from a team as a code owner April 15, 2023 02:19
@justinchuby justinchuby requested a review from snnn April 15, 2023 02:24
@justinchuby justinchuby requested review from pranavsharma and removed request for a team April 15, 2023 03:10
snnn
snnn previously approved these changes Apr 17, 2023
@justinchuby

This comment was marked as outdated.

@justinchuby justinchuby force-pushed the justinchu/clang-format branch from e641046 to 7c7e6cc Compare April 17, 2023 18:11
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

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.

Copy link
Contributor Author

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',
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

image

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/

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need a trailing comma.
image

Copy link
Contributor Author

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

@justinchuby
Copy link
Contributor Author

@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

@edgchen1
Copy link
Contributor

@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

@justinchuby
Copy link
Contributor Author

Now just

'**/*.h',
'**/*.cc',
'**/*.hpp',
'**/*.cpp',
'**/*.m',
'**/*.mm',

@snnn snnn merged commit cf19c36 into main Apr 18, 2023
@snnn snnn deleted the justinchu/clang-format branch April 18, 2023 16:27
justinchuby added a commit that referenced this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants