Repository navigation
Add gersemi CMake formatter - #6557
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
0f41e3f to
ca37156
Compare
bernhardmgruber
left a comment
There was a problem hiding this comment.
I love this for the sake of having consistent formatting!
I would also love if it wouldn't expand the code vertically so much. Looking at the changed LOCs, we expand by roughly 50%. That's a lot. Are there any options to keep the code more dense?
Dense CMake quickly becomes unreadable, and I greatly prefer readability over compactness for the sake of compactness. Newlines are cheap :) My biggest gripe about gersemi's formatting is that when it splits lines, it moves the first args to a new line, eg. message(STATUS
"Line one"
"Line two"
)becomes message(
STATUS
"Line one"
"Line two"
)I do greatly prefer the first example, but I'm willing to concede that to avoid having to think about formatting going forward 🙂 This particular refactoring is the main contributor to the increase in size. Overall, this PR adds ~2000 LOCs. Our repo has >20,000 lines of cmake total, so it's more like a 10% increase instead of 50% taken as a whole. |
dd00596 to
4aca905
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0026cc4 to
33c2823
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
miscco
left a comment
There was a problem hiding this comment.
I will gladly take any automatic formatting over me hitting whitespaces
This comment has been minimized.
This comment has been minimized.
Compactness up to some degree does strongly correlate with readability for me. But I guess this is a personal preference.
I agree! |
Yeah, that's a better reasoning. Still, if we go with 120, there is no increase :) |
|
The issue with CMake is that there is a lot of structure within those function arguments, have having them broken out makes that structure more apparent and obvious -- this is the readability gain that we get here. Having all of the args squeezed into a single line loses that structure. Maybe it's because I've spent hundreds or thousands of hours reading and writing CMake at this point, but I "just know" what eg. If all of the keywords, options, and values are compressed into a single line, it takes much more cognitive effort to read through them, mentally parse them, group them, find what I'm looking for, catch bugs...each line, each token must be inspected one-by-one, rather being able to just skim / glance, and subtle bugs like quoting errors get lost in the soup. It's a lot more work to actually read and understand long, dense lines of CMake. For these reasons I prefer the shorter, less dense lines of CMake. I'll also point out that I'm the one doing at least 98% of the CMake work for CCCL, so I'd like to ask that my opinions and subjective takes are allowed to carry a little more weight here 🙂 That said, there are some expansions I don't like and feel are unnecessary -- for instance If they decide that's worth implementing and maintaining, we'll have much more control over when expansion vs inlining occurs on a per-command basis. This will let me set up gersemi to expand the commands that are useful to have expanded, but inline those that we don't really care about (message, function, macro, etc). At this point we can revisit the line-length settings, once we're able to ensure that the commands that should be expanded for readability are never inlined. Sound like a good compromise for now? |
bdfa74c to
ce6e108
Compare
🥳 CI Workflow Results🟩 Finished in 1h 31m: Pass: 100%/263 | Total: 2d 03h | Max: 1h 04m | Hits: 99%/384652See results here. |
…VIDIA/cccl#6557) Upstream: NVIDIA/cccl@2845628 HIP counterpart (system/hip): none. N/A: gersemi formatting-only reflow of CMake files; local CMakeLists.txt/*.cmake are rocThrust-owned (HIP build) and the monorepo does not use gersemi, so no reformat ported; no functional change. test/ counterpart: none. Not compiled, tests not run (per human) Co-Authored-By: Claude <noreply@anthropic.com>
…VIDIA/cccl#6557) Upstream: NVIDIA/cccl@2845628 HIP counterpart (system/hip): none. N/A: gersemi formatting-only reflow of CMake files; local CMakeLists.txt/*.cmake are rocThrust-owned (HIP build) and the monorepo does not use gersemi, so no reformat ported; no functional change. test/ counterpart: none. Not compiled, tests not run (per human) Co-Authored-By: Claude <noreply@anthropic.com>
…VIDIA/cccl#6557) Upstream: NVIDIA/cccl@2845628 HIP counterpart (system/hip): none. N/A: gersemi formatting-only reflow of CMake files; local CMakeLists.txt/*.cmake are rocThrust-owned (HIP build) and the monorepo does not use gersemi, so no reformat ported; no functional change. test/ counterpart: none. Not compiled, tests not run (per human) Co-Authored-By: Claude <noreply@anthropic.com>
…VIDIA/cccl#6557) Upstream: NVIDIA/cccl@2845628 HIP counterpart (system/hip): none. N/A: gersemi formatting-only reflow of CMake files; local CMakeLists.txt/*.cmake are rocThrust-owned (HIP build) and the monorepo does not use gersemi, so no reformat ported; no functional change. test/ counterpart: none. Not compiled, tests not run (per human) Co-Authored-By: Claude <noreply@anthropic.com>
…VIDIA/cccl#6557) Upstream: NVIDIA/cccl@2845628 HIP counterpart (system/hip): none. N/A: gersemi formatting-only reflow of CMake files; local CMakeLists.txt/*.cmake are rocThrust-owned (HIP build) and the monorepo does not use gersemi, so no reformat ported; no functional change. test/ counterpart: none. Not compiled, tests not run (per human) Co-Authored-By: Claude <noreply@anthropic.com>
Adds configuration and precommit hooks for
gersemi, a cmake formatter:https://github.com/BlankSpruce/gersemi
I've reviewed all of the changes in this PR and am pleased overall with the results. The upstream maintainer has been responsive to questions, suggestions, and issues I've had in this process.
We have a total of 135 files with 20816 lines of CMake in our repo, and there were very few instances where I felt the need to manually format using the
# gersemi: [off|on]hints -- pretty much all of them were forCOMMANDargument formatting which I don't expect a formatter to get correct.