Conversation
|
I think that's a very good idea. |
|
That's where I've stumbled a bit to be honest @w00000dy |
w00000dy
left a comment
w00000dy
left a comment
There was a problem hiding this comment.
Since you wanted some feedback, I had a look at it. I've written what I think about a few options. The following options are suggestions for additions to the .clang-format file from my side:
InsertNewlineAtEOF: trueAllowShortBlocksOnASingleLine: true
AllowShortBlocksOnASingleLine allows this:
while (true) {}and
while (true) { continue; }| --- | ||
| BasedOnStyle: Google | ||
| AlignAfterOpenBracket: DontAlign | ||
| AlignConsecutiveDeclarations: Consecutive |
There was a problem hiding this comment.
This formats this:
float fmod_t(float num, float denom) {
int tquot = num / denom;
float res = num - tquot * denom;
return res;
}to this:
float fmod_t(float num, float denom) {
int tquot = num / denom;
float res = num - tquot * denom;
return res;
}I prefer the first version.
| AlignTrailingComments: false | ||
| AllowAllArgumentsOnNextLine: false | ||
| AllowShortCaseLabelsOnASingleLine: true | ||
| AllowShortFunctionsOnASingleLine: None |
There was a problem hiding this comment.
Why do you want to set this to None?
Now this formats this:
int add(int a, int b) { return a + b; }to this:
int add(int a, int b) {
return a + b;
}| AlignAfterOpenBracket: DontAlign | ||
| AlignConsecutiveDeclarations: Consecutive | ||
| AlignEscapedNewlines: DontAlign | ||
| AlignOperands: DontAlign |
There was a problem hiding this comment.
In the rare case that this happens, I think aligning would improve readability here.
| BasedOnStyle: Google | ||
| AlignAfterOpenBracket: DontAlign | ||
| AlignConsecutiveDeclarations: Consecutive | ||
| AlignEscapedNewlines: DontAlign |
There was a problem hiding this comment.
I think this was set because of your tool. I see no reason why we should set this to DontAlign since aligning would improve readability.
| @@ -0,0 +1,23 @@ | |||
| --- | |||
| BasedOnStyle: Google | |||
| AlignAfterOpenBracket: DontAlign | |||
There was a problem hiding this comment.
I think in the rare case where we exceed the ColumnLimit, aligning would improve readability.
| AllowShortIfStatementsOnASingleLine: Always | ||
| AlwaysBreakBeforeMultilineStrings: false | ||
| BreakBeforeTernaryOperators: false | ||
| BreakConstructorInitializersBeforeComma: true |
There was a problem hiding this comment.
The BreakConstructorInitializersBeforeComma should be replaced with the BreakConstructorInitializers option.
| ColumnLimit: 240 | ||
| ContinuationIndentWidth: 2 | ||
| IndentPPDirectives: BeforeHash | ||
| KeepEmptyLinesAtTheStartOfBlocks: true |
There was a problem hiding this comment.
This option is deprecated. See: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#keepemptylinesatthestartofblocks
| ContinuationIndentWidth: 2 | ||
| IndentPPDirectives: BeforeHash | ||
| KeepEmptyLinesAtTheStartOfBlocks: true | ||
| MaxEmptyLinesToKeep: 2 |
There was a problem hiding this comment.
Maybe we should change this so that the code doesn't take up so many lines?
| IndentPPDirectives: BeforeHash | ||
| KeepEmptyLinesAtTheStartOfBlocks: true | ||
| MaxEmptyLinesToKeep: 2 | ||
| ReflowComments: false |
There was a problem hiding this comment.
Maybe we should at least set this to IndentOnly so that misaligned comments are formatted.
| KeepEmptyLinesAtTheStartOfBlocks: true | ||
| MaxEmptyLinesToKeep: 2 | ||
| ReflowComments: false | ||
| SortIncludes: Never |
|
Thank you @w00000dy for your comments, this is my first time using clang and I'm not a C++ developer I am very grateful for input from those with experience. I'll have a look through your individual comments when I get time to do so |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
WalkthroughA new Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.clang-format (3)
3-3: Reconsider disabling bracket alignment
AlignAfterOpenBracket: DontAlignprevents aligning parameters, which can hurt readability in multi-line calls. Consider enabling alignment for consistency.
7-7: Revisit trailing comments alignment
AlignTrailingComments: falsewill leave trailing comments unaligned. ConsiderIndentOnlyto maintain comment indentation consistency.
14-14:⚠️ Potential issueDeprecated option used for constructor initializers
BreakConstructorInitializersBeforeCommais deprecated. Replace with the modernBreakConstructorInitializerssetting:-BreakConstructorInitializersBeforeComma: true +BreakConstructorInitializers: BeforeComma
🧹 Nitpick comments (2)
.clang-format (2)
8-8: AllowAllArgumentsOnNextLine consideration
AllowAllArgumentsOnNextLine: falseforbids moving all function arguments to the next line, which may reduce readability for long signatures. Consider enabling it.
13-13: Ternary operator breaks
BreakBeforeTernaryOperators: falsecompacts ternary expressions but can hinder readability for complex conditions. Consider setting it totrue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.clang-format(1 hunks).gitignore(0 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🔇 Additional comments (14)
.clang-format (14)
1-2: Baseline Style Confirmation
BasedOnStyle: Googleand the YAML header are correctly set to anchor on the Google style.
4-4: Approve consecutive declarations alignment
AlignConsecutiveDeclarations: Consecutivewill line up declarations nicely, improving readability.
5-5: Escaped newlines alignment
AlignEscapedNewlines: DontAlignis sensible to avoid unpredictable indent shifts with escaped line breaks.
6-6: Operand alignment disabled
AlignOperands: DontAlignmatches Google style and avoids uneven spacing in expressions.
9-11: Short constructs formatting
AllowShortCaseLabelsOnASingleLine: true,AllowShortFunctionsOnASingleLine: None, andAllowShortIfStatementsOnASingleLine: Alwaysalign with Google style and look appropriate.
12-12: Multiline strings behavior
AlwaysBreakBeforeMultilineStrings: falsepreserves the original formatting of long strings. This is reasonable if you want to keep manual line breaks.
15-15: String literal breaks
BreakStringLiterals: falseprevents splitting string literals. This is fine if preserving literal integrity is a priority.
16-16: Column limit sanity check
ColumnLimit: 240is much wider than typical guidelines (80–100). Confirm this is intentional for display configurations.
17-17: Continuation indent width
ContinuationIndentWidth: 2follows Google style and ensures consistent indentation for wrapped lines.
18-18: Preprocessor directive indent
IndentPPDirectives: BeforeHashis less common thanAfterHash. Verify that macro and conditional directive formatting meets expectations.
20-20: Empty lines retention
MaxEmptyLinesToKeep: 2prevents excessive blank lines and is a good default.
21-21: Comment reflow disabled
ReflowComments: falsepreserves manual comment formatting. Good if you want to control wording and layout.
22-22: Include sorting policy
SortIncludes: Neverdisables automatic include sorting. Confirm this aligns with the project's manual include grouping conventions.
23-23: Trailing comment spacing
SpacesBeforeTrailingComments: 1is standard practice and looks correct.
| ColumnLimit: 240 | ||
| ContinuationIndentWidth: 2 | ||
| IndentPPDirectives: BeforeHash | ||
| KeepEmptyLinesAtTheStartOfBlocks: true |
There was a problem hiding this comment.
Deprecated empty-lines option
KeepEmptyLinesAtTheStartOfBlocks is deprecated. Update to the current equivalent (e.g., KeepEmptyLinesAtTopOfFunctions).
🤖 Prompt for AI Agents
In the .clang-format file at line 19, replace the deprecated option
KeepEmptyLinesAtTheStartOfBlocks with the current equivalent option
KeepEmptyLinesAtTopOfFunctions to ensure compatibility with the latest
clang-format versions.
|
Please don't close this PR. I would like the idea of this PR |
willmmiles
added
keep
4 participants
Rather than just describing what code style we look for, it would be good to provide a clang-format file
To get us started I have created one based on a style detection tool and one of the current files
In order to verify if the file is right, we can run clang-format -i wled00/*.cpp and if we have the right values then all the changes should match our expectations
We might not choose to mass reformat all existing code, but it serves as useful test to check that new code being written will indeed conform to our style
I very much welcome input from others, especially those with knowledge of how to write a clang-format file
Summary by CodeRabbit