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.
There was a problem hiding this comment.
I agree - the extra spaces in int tquot - to visually align it with the definition of float fmod_t - are confusing.
| 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
WalkthroughAdded a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![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
|
@CodeRabbit please re-review. Additionally, summarize the style changes suggested in the review comments, together with a short description of what each change achieves. |
🧩 Analysis chain🏁 Script executed: Length of output: 19551
Summary of Suggested Changes to
|
| Option | Current Value | Suggested Change | Why |
|---|---|---|---|
BreakConstructorInitializersBeforeComma |
true |
Replace with BreakConstructorInitializers: BeforeComma |
The current option is deprecated (removed upstream); using it risks silent mis-formatting on newer clang-format versions. |
KeepEmptyLinesAtTheStartOfBlocks |
true |
Replace with KeepEmptyLinesAtTopOfFunctions: true (or false) |
This option is deprecated in newer clang-format releases and will be ignored without warning. |
🟠 Alignment Settings (currently too restrictive)
| Option | Current Value | Suggested Change | What it achieves |
|---|---|---|---|
AlignAfterOpenBracket |
DontAlign |
Align (or remove to use Google default) |
Aligns wrapped function arguments/parameters under the opening bracket, improving readability of long signatures. |
AlignConsecutiveDeclarations |
Consecutive |
Remove / set to None |
The current setting pads type names with spaces to vertically align identifiers in consecutive declarations (e.g., adds spaces after int to align with float). @w00000dy considers the unpadded version more readable. |
AlignEscapedNewlines |
DontAlign |
Left or Right |
Aligns the backslash \ at the end of multi-line macro definitions to a consistent column, making macros easier to read. |
AlignOperands |
DontAlign |
Align |
Aligns binary/ternary operands vertically in multi-line expressions, which improves readability when lines are broken. |
AlignTrailingComments |
false |
IndentOnly |
Ensures trailing comments (// …) are at least correctly indented even if not fully column-aligned, preventing visually misaligned comments. |
🟡 Short-construct Behavior
| Option | Current Value | Suggested Change | What it achieves |
|---|---|---|---|
AllowShortFunctionsOnASingleLine |
None |
Inline or All |
None forces even trivial one-liner functions (e.g. int add(int a, int b) { return a + b; }) to be split across multiple lines. Enabling it keeps compact functions on one line, matching common WLED style. |
AllowAllArgumentsOnNextLine |
false |
true |
Allows all arguments of an over-long call to be moved to the next line as a group, rather than breaking them individually — often cleaner for long function calls. |
BreakBeforeTernaryOperators |
false |
true |
Breaks before ? and : in ternary expressions rather than after, which many find more readable for complex conditions. |
🟢 Proposed Additions
| Option | Suggested Value | What it achieves |
|---|---|---|
InsertNewlineAtEOF |
true |
Ensures every file ends with a newline, which is required by POSIX and avoids noisy diffs. |
AllowShortBlocksOnASingleLine |
true (or Empty) |
Allows empty or trivially-short blocks (e.g. while (true) {}) to remain on a single line, matching current WLED style. |
⚪ Open Questions
| Option | Current Value | Question raised |
|---|---|---|
MaxEmptyLinesToKeep |
2 |
@w00000dy suggests potentially lowering this to 1 to keep code less sprawling. |
ReflowComments |
false |
@w00000dy suggests IndentOnly as a middle ground: doesn't reflow comment text but still fixes indentation of misaligned comment lines. |
SortIncludes |
Never |
@w00000dy questions whether this should remain disabled — enabling it (true or IncludeBlocks: Regroup) would enforce consistent #include ordering but could disrupt manually ordered includes in WLED (e.g. platform-specific guards). This is a project-level policy decision. |
ColumnLimit |
240 |
Notably wide (typical is 80–120). Confirmed as intentional for WLED given wide monitor usage, but worth documenting explicitly. |
Triggering a fresh re-review now.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T11:41:46.737Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-12T07:15:56.657Z
Learning: Match existing code style in files you edit; C++ formatting is available via `clang-format` but is not in CI
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-12T07:15:56.657Z
Learning: Document changes in pull requests with clear description of what changed and why, including expected impact on user-visible behavior, related issue links, and screenshots for new features
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.clang-format (1)
14-19:⚠️ Potential issue | 🟠 MajorReplace deprecated clang-format keys (tooling compatibility risk).
Line 14 and Line 19 use deprecated options. Please migrate to current keys so this file stays valid across newer clang-format versions.
🔧 Proposed fix
-BreakConstructorInitializersBeforeComma: true +BreakConstructorInitializers: BeforeComma @@ -KeepEmptyLinesAtTheStartOfBlocks: true +KeepEmptyLinesAtTopOfFunctions: trueIn current clang-format documentation, is `BreakConstructorInitializersBeforeComma` deprecated in favor of `BreakConstructorInitializers`? Also, is `KeepEmptyLinesAtTheStartOfBlocks` deprecated in favor of `KeepEmptyLinesAtTopOfFunctions`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.clang-format around lines 14 - 19, Replace the two deprecated clang-format options: change BreakConstructorInitializersBeforeComma to the modern BreakConstructorInitializers and preserve the choice (e.g., BeforeComma) as its value, and change KeepEmptyLinesAtTheStartOfBlocks to KeepEmptyLinesAtTopOfFunctions keeping the boolean value; update these keys in the file where the existing deprecated options appear so the config uses BreakConstructorInitializers and KeepEmptyLinesAtTopOfFunctions instead of the old names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.clang-format:
- Around line 14-19: Replace the two deprecated clang-format options: change
BreakConstructorInitializersBeforeComma to the modern
BreakConstructorInitializers and preserve the choice (e.g., BeforeComma) as its
value, and change KeepEmptyLinesAtTheStartOfBlocks to
KeepEmptyLinesAtTopOfFunctions keeping the boolean value; update these keys in
the file where the existing deprecated options appear so the config uses
BreakConstructorInitializers and KeepEmptyLinesAtTopOfFunctions instead of the
old names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90629ca7-779f-4696-84a4-e20ea90fca7d
📒 Files selected for processing (2)
.clang-format.gitignore
💤 Files with no reviewable changes (1)
- .gitignore
5 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