STY: clang-format repo #5
Conversation
lucascolley
force-pushed
the
format
branch
2 times, most recently
from
244f6de to
12758ea
Compare
|
@steppi I know there are more important things for you to prioritise, but it would be great if you could check whether any of the formatting configuration looks unacceptably bad here. I think that even getting a non-ideal configuration merged is a big step up from having none at all, as that will allow us to enforce it in CI and make all future formatting automatic. |
|
the diff is scarily large just due to |
.clang-format
Outdated
| ColumnLimit: 120 | ||
| InsertNewlineAtEOF: true | ||
| AlignAfterOpenBracket: BlockIndent | ||
| IncludeBlocks: Preserve | ||
| AllowShortFunctionsOnASingleLine: None |
There was a problem hiding this comment.
I seem to remember that @izaid deliberately chose a style where short functions could be on a single line. Is there a reason we are changing this now? It mostly effects wrappers like those appearing in config.h.
There was a problem hiding this comment.
If I remember correctly, this came from discussions with @fancidev on the superseded PR to SciPy. It was a while ago, though, so not sure
There was a problem hiding this comment.
you can take a look at it without adding this option now
There was a problem hiding this comment.
I see. From here, scipy/scipy#21640 (comment), it looks like @fancidev only wanted to avoid having short functions automatically converted to one line, but didn't want existing one line wrappers to become split across multiple lines. That's good, it seems everyone is actually in agreement about not including AllowShortFunctionsOnASingleLine: None. I didn't want to change something that @izaid had explicitly chosen during a period of time where he doesn't have time to participate in discussions.
| int acai(std::complex<double>, double, int, int, int, std::complex<double> *, double, double, double, double); | ||
| int | ||
| acon(std::complex<double>, double, int, int, int, std::complex<double> *, double, double, double, double, double); | ||
| int asyi(std::complex<double>, double, int, int, std::complex<double> *, double, double, double, double); |
There was a problem hiding this comment.
It seems a little noisy that clang-format will sometimes put the return type on the previous line and sometimes on the same line, in order to keep from exceeding the max column width. I think we should just pick one style and be consistent. Can you see how it looks after setting BreakAfterReturnType in .clang-format to None?
There was a problem hiding this comment.
Huh, that didn't work though. There's still a mix.
There was a problem hiding this comment.
I see, None is deprecated, and now it does something called Automatic that sometimes does and sometimes doesn't break depending on a penalty calculation. I don't want to fight clang-format so let's just take out BreakAfterReturnType.
|
if you would like to test other config options locally, you can do so with |
|
I made a final update to the clang-format config, cleaned up history with an interactive rebase, and added a commit to add the clang-format commit to |
|
thanks Albert! Now lets get the submodule PR in before cherry-picking becomes a nightmare 😅 |
Continues scipy/scipy#21640.
cc @fancidev