search

ComputationalRadiationPhysics / contributing

:books: guidelines and styles to contribute to our projects
Creative Commons Attribution 4.0 International
6 stars 10 forks source link

Update to clang-format-16 #66

Closed j-stephan closed 1 year ago

j-stephan commented 1 year ago

It has been a while since we updated the clang-format style. This PR adds the rules added to clang-format since the last overhaul and removes those which are deprecated. I tried to follow the existing (manual) alpaka style where possible but we should vote on the changes.

New rules (value after name = current proposed setting):

Replacements:

New project-specific options:

bernhardmgruber commented 1 year ago

BreakBeforeInlineASMColon: I would go with Never or OnlyMultiline until we find a problem. InsertBraces: I am super strongly against this one, because it just creates textual noise. Even worse, because alpaka already places each brace on a separate line. IntegerLiteralSeparator: that's a fun one. I am curious how this will turn out. PenaltyBreakOpenParentheses and PenaltyIndentedWhitespace: how do you come up with these numbers? why not just leave the defaults? PPIndentWidth: again, just leave the default (which is -1). RemoveBracesLLVM: as much as would like to see this as true, I know that older nvcc versions sometimes have parse errors when braces are missing on the else branch of an if constexpr. RequiresExpressionIndentation: I am leaning towards OuterScope here

j-stephan commented 1 year ago

BreakBeforeInlineASMColon: I would go with Never or OnlyMultiline until we find a problem.

I find inline ASM statements already hard to parse because we don't use them very often. Having them in multiple lines makes them easier to read IMO.

PenaltyBreakOpenParentheses and PenaltyIndentedWhitespace: how do you come up with these numbers? why not just leave the defaults?

It isn't documented what the default is here. I just used the value from PenaltyBreakAssignment since that seems to work well.

PPIndentWidth: again, just leave the default (which is -1).

Just making the default explicit here.

SimeonEhrig commented 1 year ago

InsertBraces: I would prefer true. I had already the problem in the past, that I add a second line to a if body and forgot to add the braces. Have fun to find the strange behavior.

bernhardmgruber commented 1 year ago

InsertBraces: I would prefer true. I had already the problem in the past, that I add a second line to a if body and forgot to add the braces. Have fun to find the strange behavior.

But you immediately notice that since, either, clang-format moves it, or clang warns you about a missleading indentation.

SimeonEhrig commented 1 year ago

But only, if you integrated it in your IDE. No one is running clang-format manually after code change.

bernhardmgruber commented 1 year ago

But only, if you integrated it in your IDE. No one is running clang-format manually after code change.

I actually do both. CLion formats while a type, but sometimes I also do changes inside the diff viewer, or from the command line, in which case I run git clang-format :)

j-stephan commented 1 year ago

I'm a bit conflicted on the brace issue. I dislike it for this:

if(cond)
    one_liner_expression;

but would want it for this:

while (i--) 
  for (auto *A : D.attrs())
      handleAttr(A);

Unfortunately we cannot configure this beyond true or false so I'm slightly for inserting braces...

bernhardmgruber commented 1 year ago

@j-stephan given the two examples you listed, inserting braces will

if(cond)
    one_liner_expression;

double the screen space required for this piece of code. And for

while (i--) 
  for (auto *A : D.attrs())
      handleAttr(A);

We add even more noise, since we turn 3 lines to 7. That is a crazy blow up of source code.

psychocoderHPC commented 1 year ago

I am against adding braces for single-line expressions. I know that there is the possibility to introduce errors during refactoring but adding braces is easily adding many lines per file.

j-stephan commented 1 year ago

Okay. So we are leaning towards InsertBraces: false. If there are no other wishes by the majority I'll change the line and then go ahead and merge this PR.

psychocoderHPC commented 1 year ago

BreakBeforeInlineASMColon: I am for OnlyMultiline IntegerLiteralSeparator I do not like it or do not see why we need it. RequiresExpressionIndentation' I am forOuterScopebecause the other option looks like an alignment and we decided in the past to indent always and never align but optionKEyword` is aligning with the keyword.

j-stephan commented 1 year ago

I made the following changes: