-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Style: Add 19.1.0 LLVM options to .clang-format
#99548
Style: Add 19.1.0 LLVM options to .clang-format
#99548
Conversation
## Godot TODO: We'll want to use a min of 1, but we need to see how to fix | ||
## our comment capitalization at the same time. | ||
SpacesInLineCommentPrefix: | ||
Minimum: 0 | ||
Minimum: 0 # We want a minimum of 1 for comments, but allow 0 for disabled code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has been around for a while, and it isn't exactly relevant when an offset of 0 is desired for disabled code. I've replaced it with one on the same line to reflect current context. If we ever decide to handle disabled code a different way, this whole section can be commented out, as LLVM uses a minimum of 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's tricky, for commented out code I think we should aim to:
- Not rely on having commented out code in the first place
- For the rare cases where it's relevant, use either
#if 0
or/* */
multiline comment blocks.
And then we could start enforcing one space after //
for comments.
That being said, as our PR backlog keeps growing, I'm increasingly against codebase wide tweaks like this which add conflicts and frictions in lots of PRs. I think our priorities need to be on reviewing and merging PRs, and once we've reduced the backlog significant and feel like we have time to spare, we can do cosmetic passes like tweaking comment formatting.
In general, for this kind of stuff I think the better approach with our scale is to introduce new policies but not apply them to the whole codebase if they're too invasive, and just make them a soft requirement for new code (like we've been doing with the comment style, but I'm seeing pushback from some maintainers that even that might be too contributor adverse for new contributors specifically - maintainers can learn and adopt a new style).
Docs will need to be updated to reflect the new minimum version https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#using-clang-format-locally |
Good point, though those guidelines are partially irrelevant now, but I'm not sure how to update them. 'pre-commit` takes care of everything, including installing that exact version of clang-format. So the instructions for how to install clang-format manually are redundant and misleading. But they're still partially relevant for folks who'd like to setup clang-format in their IDE to have formatting on save and not just on commit. But this becomes more of an advanced use case than what should be our recommendation to all contributors. |
I second this as these changes break compatibility with clang-format 18.1.3 (this is the version I have, shipped by default on ubuntu 24.04). As usual when the parsing of .clang-format file fails, This took me by surprise this morning when I synced my fork. |
Given
pre-commit-config.yaml
has already bumped clang-format from 18.1.8 to 19.1.0, it was only fair to give the same treatment to our.clang-format
file. Not too much new was added, seemed to mostly be renames & features for other languages, but one option stood out that made sense to apply right away:KeepEmptyLines.AtStartOfFile: false
.KeepEmptyLines
is now a container for all options of that type, with the two options migrating to it (KeepEmptyLinesAtEOF
→KeepEmptyLines.AtEndOfFile
,KeepEmptyLinesAtTheStartOfBlocks
→KeepEmptyLines.AtStartOfBlock
) being ones we already disable, so this simply makes the new option match our expected behavior. Only a handful of files were affected, none of which were depending on that behavior, so it's an entirely safe conversion.