Skip to content
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

use clang-format in hiop to keep code style #700

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Conversation

nychiang
Copy link
Collaborator

Use clang-format in hiop to keep code style.

@cnpetra

@cnpetra cnpetra requested review from tepperly and cnpetra November 27, 2024 16:46
@tepperly
Copy link
Member

You've asked for my review. I think adding style enforcement & checking using clang-format is a great decision. For LiDO, we ended up needing to install the same version of clang-format on all our testing platforms because there are some changes in the output for different versions of clang-format.

Of course, formatting is something that everyone has strong, personal opinions about. Your choices aren't the same as mine or LiDO's, but I think they're very reasonable choices. I prefer formats like your that don't put curly braces on their own line except for the outer braces for a method/function definition. For me, putting curly braces on their own line stretches everything vertically, and monitors typically have more horizontal space than vertical space.

I think having consistent formatting is a positive rather than each file or area having its own style. I don't think that I should ocmment on your individual choices because it should match your aesthetic. If you want, I can send you LiDO's .clang-format, but I think what you have is fine.

Copy link
Member

@tepperly tepperly left a comment

Choose a reason for hiding this comment

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

My only other advice is when/if you change your .clang-format, it should be the only thing happening in the PR (i.e., don't mix changing the format with other coding changes). The formatting will change so many files that the other code changes will either slip under the radar or it will force the reviewer to carefully scan through all the changes looking for the substantial ones.

@nychiang
Copy link
Collaborator Author

@tepperly Thanks for your quick response! I totally agree that we should avoid introducing other changes in the PR related to code formatting.
In this .clang-format configuration, I've specifically limited changes to whitespace adjustments and explicitly disabled any modifications that could alter the code's functionality—such as sorting include head files or adding braces to if/for statements. As a result, this PR should be safe in terms of functionality.

Moving forward, we plan to implement a CI pipeline to check code style without making any automatic changes to the code itself.

@nychiang nychiang marked this pull request as ready for review November 27, 2024 22:34
@tepperly
Copy link
Member

@tepperly Thanks for your quick response! I totally agree that we should avoid introducing other changes in the PR related to code formatting. In this .clang-format configuration, I've specifically limited changes to whitespace adjustments and explicitly disabled any modifications that could alter the code's functionality—such as sorting include head files or adding braces to if/for statements. As a result, this PR should be safe in terms of functionality.

I think we sort #include lines except that every foo.cpp must #include "foo.hpp" first if such a file exists. This forces foo.hpp to be self-sufficient. It has to work when it's the first one #included.

Moving forward, we plan to implement a CI pipeline to check code style without making any automatic changes to the code itself.

I think it's good to have a style check CI pipeline.

@tepperly
Copy link
Member

If you use blt along with CMake, you can specify the style checking approach. Testing compliance clang-format is a matter of seeing if make check has a clear return status. You can apply the style rules with a make style.

@nychiang
Copy link
Collaborator Author

@tepperly yes, I have already got some discussion with Chris. We are using a very old version of BLT in HiOP now, and I plan to update BLT in another PR. Cheers!

@cnpetra cnpetra merged commit 8c0e7b9 into develop Dec 2, 2024
6 checks passed
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.

3 participants