STY: xsf: apply clang-format #21640
STY: xsf: apply clang-format #21640
Conversation
github-actions
bot
added
scipy.special
C/C++
lucascolley
changed the title
There was a problem hiding this comment.
Looks good to me overall. I just leave a few comments about my personal preference.
Can I ask what is the "screen width" configured for the auto format? It seems to have an impact on the wrappings done. (I'm personally a fan of 80 width, but I understand that many people prefer wider.)
scipy/special/xsf/binom.h
Outdated
| XSF_HOST_DEVICE inline float binom(float n, float k) { | ||
| return binom(static_cast<double>(n), static_cast<double>(k)); | ||
| } | ||
| XSF_HOST_DEVICE inline float binom(float n, float k) { return binom(static_cast<double>(n), static_cast<double>(k)); } |
There was a problem hiding this comment.
I'd prefer to leave single-line function body as is than to merging it in the same line as function signature, so that if we add more lines into the function body later, we don't have to "undo" the merge and cause spurious diff.
There was a problem hiding this comment.
Do you know how to change the config file to get what you are looking for?
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for checking. It seems the new option would convert an existing single-line function body into multiple lines. I feel this is unnecessary if the function body is indeed a simple wrapper.
It would have been best if clang-format provided a "preserve" option that leaves single-line/multi-line function body unchanged, but seems there's no such option. I think I like the previous behavior better (i.e. merging short one-liner into the line of {) as it creates fewer "bad" choices (according to my preference) than the new option.
| } | ||
| m = 20 + (int)(80.0/x); | ||
| m = 20 + (int) (80.0 / x); |
There was a problem hiding this comment.
I'd prefer not to insert a blank between (int) and (80.0 / x), because (int) (reinterpret cast) acts like a function, and function names are usually not separated from the opening parenthesis. Similarly e.g. for (std::max)(1.0, 2.0).
There was a problem hiding this comment.
Do you know how to change the config file to get what you are looking for?
There was a problem hiding this comment.
Is this an instance of SpaceAfterCStyleCast: true?
There was a problem hiding this comment.
Yes. Setting it to false should achieve what I have in mind.
There was a problem hiding this comment.
Can we hold up guys? I'm not keen on this particular change, for instance. More broadly, there is a plan to actually move xsf out of special and I think that is likely to happen soon.
| -9.0954017145829042233E-2, 1.0009945751278180853E-1, -1.1133426586956469049E-1, 1.2550966952474304242E-1, | ||
| -1.4404989676884611812E-1, 1.6955717699740818995E-1, -2.0738555102867398527E-1, 2.7058080842778454788E-1, | ||
| -4.0068563438653142847E-1, 8.2246703342411321824E-1, -5.7721566490153286061E-1}; | ||
| double coeffs[] = {-4.3478266053040259361E-2, 4.5454556293204669442E-2, -4.7619070330142227991E-2, |
There was a problem hiding this comment.
Why does clang-format move the first line of numbers into the same line as { here? I noticed that in some places clang-format leaves the numbers in their own line. Do you by chance know what is the rule?
There was a problem hiding this comment.
Maybe Cpp11BracedListStyle: true
Yes I was planning to add that at the end.
I believe @izaid chose |
|
There's a format file in xsf folder: https://github.com/scipy/scipy/blob/main/scipy/special/xsf/.clang-format |
Yep, that's what I'm using here. |
lucascolley
force-pushed
the
clang-format-xsf
branch
from
11188fd to
791fc53
Compare
|
Continued at scipy/xsf#5 |
Reference issue
A more sensible first step towards gh-21024.
What does this implement/fix?
Apply clang-format to
scipy/special/xsf, excluding/third_party.Additional information
It was a bit ambitious to try to start applying clang-format across the repo immediately. As noted in #21024 (comment), we should probably only do this for non-legacy (and non-vendored) code.
There are a few potential complications (#21024 (comment)), so easier to take this slow and steady. Here is a start for just
xsf.Hopefully this PR will be a good point to make sure we are happy with the config before applying it elsewhere.
cc @izaid @steppi