ci: add clang-format to pre-commit configuration #2902
Conversation
|
Wow the pre-commit bot did a really big diff, is the clang-format being ran correctly and it wasn't formatted before or what is going on? I'm having troubles running this |
lobis
force-pushed
the
pre-commit-clang-format
branch
from
673ee43 to
604d252
Compare
|
Not sure why this is such a big diff @agoose77 , perhaps you want to make some changes? |
|
This was something that @ianna and @jpivarski were discussing a while ago. I don't recall if we ever reached a conclusion on the style and went as far as enforcing it. Perhaps they can chime in? |
| const int8_t kMaxInt8 = 127; // 2**7 - 1 | ||
| const uint8_t kMaxUInt8 = 255; // 2**8 - 1 | ||
| const int32_t kMaxInt32 = 2147483647; // 2**31 - 1 | ||
| const uint32_t kMaxUInt32 = 4294967295; // 2**32 - 1 | ||
| const int64_t kMaxInt64 = 9223372036854775806; // 2**63 - 2: see below | ||
| const int64_t kSliceNone = kMaxInt64 + 1; // for Slice::none() | ||
| const int64_t kMaxLevels = 48; | ||
| const int8_t kMaxInt8 = 127; // 2**7 - 1 | ||
| const uint8_t kMaxUInt8 = 255; // 2**8 - 1 | ||
| const int32_t kMaxInt32 = 2147483647; // 2**31 - 1 | ||
| const uint32_t kMaxUInt32 = 4294967295; // 2**32 - 1 | ||
| const int64_t kMaxInt64 = 9223372036854775806; // 2**63 - 2: see below | ||
| const int64_t kSliceNone = kMaxInt64 + 1; // for Slice::none() | ||
| const int64_t kMaxLevels = 48; |
There was a problem hiding this comment.
I liked the right-formatting of these numbers, but it's impressive that it exactly maintained the alignment of the comments (which is more important, anyway).
| ERROR awkward_BitMaskedArray_to_IndexedOptionArray( | ||
| T* toindex, | ||
| const uint8_t* frombitmask, | ||
| int64_t bitmasklength, | ||
| bool validwhen, | ||
| bool lsb_order) { | ||
| ERROR | ||
| awkward_BitMaskedArray_to_IndexedOptionArray(T* toindex, | ||
| const uint8_t* frombitmask, | ||
| int64_t bitmasklength, | ||
| bool validwhen, | ||
| bool lsb_order) { |
There was a problem hiding this comment.
This kind of indentation is bad for limited horizontal space. Also, there's a reason black chose to put all of the arguments on a different line from the function name: with this formatting, changing the length of the name would change the indentation of all of the arguments, which is a bigger, more noisy change.
If it's possible to ask for black-style indentation here, that would be an improvement.
| if ((byte & ((uint8_t)1)) == validwhen) { | ||
| toindex[i*8 + 0] = i*8 + 0; | ||
| } | ||
| else { | ||
| toindex[i*8 + 0] = -1; | ||
| toindex[i * 8 + 0] = i * 8 + 0; | ||
| } else { | ||
| toindex[i * 8 + 0] = -1; |
There was a problem hiding this comment.
I disagree with } else {, but not strongly.
| #define FILENAME(line) FILENAME_FOR_EXCEPTIONS_C("src/cpu-kernels/awkward_ByteMaskedArray_getitem_nextcarry_outindex.cpp", line) | ||
| #define FILENAME(line) \ | ||
| FILENAME_FOR_EXCEPTIONS_C( \ | ||
| "src/cpu-kernels/" \ | ||
| "awkward_ByteMaskedArray_getitem_nextcarry_outindex.cpp", \ | ||
| line) |
There was a problem hiding this comment.
These lines were easier to edit and check in bulk when they were one-liners, but it could be hard to explain to the formatter that only macros are allowed to violate the line width rule.
Before this PR, awkward-cpp/src/cpu-kernels/awkward_ListOffsetArray_reduce_nonlocal_outstartsstops_64.cpp already had a multi-line implementation. That was the only file, so the pattern was already broken (automated scans that assumed that it's one line would have failed to catch that kernel).
So I guess this is fine. At least it's enforced.
|
@ianna and I had discussed it in #1842, #1865, #34, and #35. This does have a lot of diffs, mostly in whitespace. We can merge a PR like this, but only once, after conclusively deciding on a set of style options. Clang-format/clang-tidy doesn't come with a set of opinions, the way that black does, but there must be some de facto standards, like LLVM Style. Which is the most popular one? |
|
The issue, #1842, is still open, and we should try again someday. This (closed) PR will be helpful for whoever tries to do it in the future. |
No description provided.