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

[WIP] pre-commit: set up clang-format hook #5687

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

EZoni
Copy link
Member

@EZoni EZoni commented Feb 19, 2025

This PR introduces a prototype for adding the clang-format hook to pre-commit.

To disable formatting, e.g., on blocks of mathematical formulas, use:

// clang-format off
amrex::Real x = my     * custom
              + pretty * alignment;
// clang-format on

Currently, the hook is applied only to the Source/main.cpp file to demonstrate its functionality.

If this approach is deemed useful, we can gradually extend it to all C++ files in our codebase, one PR at a time. We could make this into a GitHub "project" to easily keep track of the progress. If not, please feel free to close the PR without merging.

The .clang-format configuration file has been generated based on the LLVM style using the command

clang-format -style=llvm -dump-config > .clang-format

and has been modified in the following ways:

  • AlwaysBreakAfterDefinitionReturnType: All  # instead of None
    
  • IndentWidth: 4  # instead of 2
    
  • PointerAlignment: Left  # instead of Right
    
  • SpaceBeforeParens: Custom  # instead of ControlStatements
    SpaceBeforeParensOptions:
      ...
      AfterFunctionDefinitionName: true  # instead of false
      AfterFunctionDeclarationName: true  # instead of false
      ...
    

A different base style could be chosen and/or further customization could be done in future PRs as needed, when the formatting is applied to more code.

@EZoni EZoni added component: tests Tests and CI cleaning Clean code, improve readability labels Feb 19, 2025
@EZoni EZoni requested review from ax3l and lucafedeli88 February 19, 2025 19:26
Source/main.cpp Outdated
warpx::initialization::initialize_external_libraries(argc, argv);
{
WARPX_PROFILE_VAR("main()", pmain);

auto timer = ablastr::utils::timer::Timer{};
timer.record_start_time();

auto& warpx = WarpX::GetInstance();
auto &warpx = WarpX::GetInstance();
Copy link
Member

@dpgrote dpgrote Feb 19, 2025

Choose a reason for hiding this comment

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

Having a formatting standard is a good idea - thanks!
However, my own preference is to have a space before and after the ampersand, since to me it is easier to read.

auto & warpx = WarpX::GetInstance();

In .clang-format, this would be

ReferenceAlignment: Middle

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @dpgrote! This is exactly the kind of feedback process I was hoping for regarding this formatting effort. I am personally fine with your preference, so I implemented the change. Others can chime in as well.

@EZoni
Copy link
Member Author

EZoni commented Feb 20, 2025

We could also run the new clang-format hook on more complex files in this PR, e.g., WarpX.H and WarpX.cpp, and see what other rules we would like to customize before we move forward with the rest of the codebase in separate PRs.

Copy link
Member

@roelof-groenewald roelof-groenewald left a comment

Choose a reason for hiding this comment

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

I agree having a standard formatting would be great. The rules set so far look good to me and, as you said, we can refine as we go.

Copy link
Member

@lucafedeli88 lucafedeli88 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @EZoni ! I agree with @roelof-groenewald that having standard formatting is good.

About the rules that you have set, I have to confess that I am not a big fan of ReferenceAlignment: Middle , because when I see something likeA & B I immediately think or "A logical_and B" rather than "B is a reference to type A".
My personal preference on this point is left alignment, because when I see A& I immediately think "type is reference to A". However, I agree that it is a bit less visible and I am OK with the other options for "ReferenceAlignment" if other maintainers prefer them.

Concerning the other rules that you have set, I like them!

@EZoni
Copy link
Member Author

EZoni commented Feb 20, 2025

About the rules that you have set, I have to confess that I am not a big fan of ReferenceAlignment: Middle , because when I see something likeA & B I immediately think or "A logical_and B" rather than "B is a reference to type A".
My personal preference on this point is left alignment, because when I see A& I immediately think "type is reference to A". However, I agree that it is a bit less visible and I am OK with the other options for "ReferenceAlignment" if other maintainers prefer them.

Thanks, @lucafedeli88! If there are conflicting opinions on ReferenceAlignment perhaps we can have an internal vote in the technical committee.

Note that the default LLVM style recommends A &B = C, which matches the alignment they recommend for pointers A *B = C. In fact, they set ReferenceAlignment = Pointer and PointerAlignment = Right, by default.

@WeiqunZhang
Copy link
Member

WarpX(development)$ git grep "[[:alnum:]]\s&[[:alnum:]]" Source/ | wc -l
271

WarpX(development)$ git grep "[[:alnum:]]\s&\s[[:alnum:]]" Source/ | wc -l
828

WarpX(development)$ git grep "[[:alnum:]]&\s[[:alnum:]]" Source/ | wc -l
3966

@EZoni
Copy link
Member Author

EZoni commented Feb 20, 2025

Also interesting for more context, from Bjarne Stroustrup's C++ Style and Technique FAQ: https://www.stroustrup.com/bs_faq2.html#whitespace.

@EZoni EZoni changed the title pre-commit: set up clang-format hook [WIP] pre-commit: set up clang-format hook Feb 20, 2025
@ax3l
Copy link
Member

ax3l commented Feb 20, 2025

Not sure if I shared this before, but we have clang-format integration in openPMD-api as a GH workflow & pre-commit hook:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleaning Clean code, improve readability component: tests Tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants