Updates .clang-format file to reflect latest supported Clang-Format standards while preserving most of the existing formatting behavior.
Note that AfterStruct brace placement was originally aligned here with AfterClass, but was reverted by reviewer demand.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32813.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, maflcko, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
44 | SpacesInCStyleCastParentheses: false 45 | SpacesInParentheses: false 46 | -BreakBeforeConceptDeclarations: Always 47 | -RequiresExpressionIndentation: OuterScope 48 | -Standard: c++20 49 | -UseTab: Never
a666d8ab6f3c019a9f7e184beb9b90affbb8075c: not sure about removing the default values. I don't think there is any harm in having them. In fact, we don't want this to silently change from down under with the next clang update.
What about the other default values, what if those change?
I'd say the others are probably edge cases to not matter much, but no strong opinion. Seems fine to add them as well to avoid silent changes. However, I don't think the config is forward compatible, so it can probably only contain keys of the minimum required clang version.
I've added all of them, this was the simplest change with fewest commits and explanations
Again, as said in my previous comment this doesn't work, because older clang-formats will just error out:
$ clang-format-16 -i src/bench/prevector.cpp
/b-c/src/.clang-format:12:3: error: unknown key 'AlignFunctionPointers'
AlignFunctionPointers: false
^~~~~~~~~~~~~~~~~~~~~
Error reading /b-c/src/.clang-format: Invalid argument
Thanks, managed to install an older version and redid the changes for maximal compatibility.
0 | @@ -1,26 +1,40 @@ 1 | AccessModifierOffset: -4 2 | -AlignAfterOpenBracket: true 3 | +AlignAfterOpenBracket: Align
7dda3d2c0e65ac1fa08ac3b24a13f2005c6bef1a: not sure about changing the values. seems fine to just keep the old value, which will likely work forever in a backward compatible way
Many of these keys aren't even listed in the documentation and the IDE shows that many are invalid since it doesn't match the allowed schema.
last commit seems fine, but the others i am not sure. at a minimum you'll have to drop the scripted diffs from executing, because they can't be reproduced anyway with different clang-format versions
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/44780552247</sub>
<sub>LLM reason (✨ experimental): The CI failure is caused by errors in the lint check, related to code style or formatting issues.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
0 | @@ -1,49 +1,222 @@ 1 | Language: Cpp 2 | AccessModifierOffset: -4 3 | -AlignAfterOpenBracket: true 4 | -AlignEscapedNewlinesLeft: true 5 | -AlignTrailingComments: true 6 | +AlignAfterOpenBracket: Align
in 7c9b3e1eae8f206753457149f1b1c837f6627d6d: How to reproduce the commit on non-macos? I don't have an apple, so I tried:
# clang-format-16 --version
Ubuntu clang-format version 16.0.6 (23ubuntu4)
However, the command:
clang-format-16 -dump-config > .clang-format
produces a different result:
# git diff -U0
diff --git a/src/.clang-format b/src/.clang-format
index 2f3f96ae2e..0822db5d11 100644
--- a/src/.clang-format
+++ b/src/.clang-format
@@ -0,0 +1 @@
+---
@@ -2 +3,2 @@ Language: Cpp
-AccessModifierOffset: -4
+# BasedOnStyle: LLVM
+AccessModifierOffset: -2
@@ -29 +31 @@ AlignConsecutiveMacros:
-AlignEscapedNewlines: Left
+AlignEscapedNewlines: Right
@@ -37 +39 @@ AllowShortBlocksOnASingleLine: Never
-AllowShortCaseLabelsOnASingleLine: true
+AllowShortCaseLabelsOnASingleLine: false
@@ -40 +42 @@ AllowShortFunctionsOnASingleLine: All
-AllowShortIfStatementsOnASingleLine: WithoutElse
+AllowShortIfStatementsOnASingleLine: Never
@@ -46 +48 @@ AlwaysBreakBeforeMultilineStrings: false
-AlwaysBreakTemplateDeclarations: Yes
+AlwaysBreakTemplateDeclarations: MultiLine
@@ -54 +56 @@ BraceWrapping:
- AfterClass: true
+ AfterClass: false
@@ -58 +60 @@ BraceWrapping:
- AfterFunction: true
+ AfterFunction: false
@@ -61 +63 @@ BraceWrapping:
- AfterStruct: true
+ AfterStruct: false
@@ -76 +78 @@ BreakBeforeConceptDeclarations: Always
-BreakBeforeBraces: Custom
+BreakBeforeBraces: Attach
@@ -78 +80 @@ BreakBeforeInlineASMColon: OnlyMultiline
-BreakBeforeTernaryOperators: false
+BreakBeforeTernaryOperators: true
@@ -82 +84 @@ BreakStringLiterals: true
-ColumnLimit: 0
+ColumnLimit: 80
@@ -123 +125 @@ IndentRequiresClause: true
-IndentWidth: 4
+IndentWidth: 2
@@ -137 +139 @@ JavaScriptWrapImports: true
-KeepEmptyLinesAtTheStartOfBlocks: false
+KeepEmptyLinesAtTheStartOfBlocks: true
@@ -142 +144 @@ MacroBlockEnd: ''
-MaxEmptyLinesToKeep: 2
+MaxEmptyLinesToKeep: 1
@@ -160 +162 @@ PenaltyReturnTypeOnItsOwnLine: 60
-PointerAlignment: Left
+PointerAlignment: Right
@@ -208 +210 @@ SpacesInSquareBrackets: false
-Standard: c++20
+Standard: Latest
@@ -222,0 +225 @@ WhitespaceSensitiveMacros:
+
Many of these keys aren't even listed in the documentation and the IDE shows that many are invalid since it doesn't match the allowed schema.
How many are those? Maybe just fixing them up manually one-by-one, if they are relevant is enough?
How to reproduce the commit on non-macos
Please see the commit messages - should I move the instructions to the PR description as well?
produces a different result
I think you have dumped the defaults that don't load our config first, which are indeed different from what we have (hence our own config).
$ clang-format-16 --version
Ubuntu clang-format version 16.0.6 (23ubuntu4)
$ clang-format-16 -dump-config | grep AfterClass
AfterClass: false
$ clang-format-16 -dump-config -style=file:src/.clang-format | grep AfterClass
AfterClass: true
and
$ clang-format-16 -dump-config -style=file:src/.clang-format | egrep 'AccessModifierOffset|AlignEscapedNewlines|AllowShortCaseLabelsOnASingleLine|AllowShortIfStatementsOnASingleLine|AlwaysBreakTemplateDeclarations|AfterClass|AfterFunction|AfterStruct|BreakBeforeBraces|BreakBeforeTernaryOperators|ColumnLimit|IndentWidth|KeepEmptyLinesAtTheStartOfBlocks|MaxEmptyLinesToKeep|PointerAlignment|Standard'
AccessModifierOffset: -4
AlignEscapedNewlines: Left
AllowShortCaseLabelsOnASingleLine: true
AllowShortIfStatementsOnASingleLine: WithoutElse
AlwaysBreakTemplateDeclarations: Yes
AfterClass: true
AfterFunction: true
AfterStruct: false
BreakBeforeBraces: Custom
BreakBeforeTernaryOperators: false
ColumnLimit: 0
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
DerivePointerAlignment: false
IndentWidth: 4
KeepEmptyLinesAtTheStartOfBlocks: false
MaxEmptyLinesToKeep: 2
ObjCBlockIndentWidth: 2
PointerAlignment: Left
PPIndentWidth: -1
AfterFunctionDefinitionName: false
AfterFunctionDeclarationName: false
Standard: c++20
clang-format-16 -dump-config > .clang-format
to answer my own question, I guess redirecting > is the problem here. It works fine when redirecting to a tmp file first: ( clang-format-16 -dump-config > tmp ) && mv tmp ./.clang-format
I haven't checked if this fixes the issues with your IDE, but I guess it makes sense either way to get the clang-format config up to date. clang-16 is the minimum required for about a year now, so every developer should have access to clang-format-16 in some way or another.
I've recreated the commits on Ubuntu and reviewed the diff:
review ACK 94364a7d5403d9d480ebc065f8709c6dd21e543f 🥙
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 94364a7d5403d9d480ebc065f8709c6dd21e543f 🥙
xsnBRKa1+85+2JOjTRQ1kZgSlBHhVZxj7LlQpMQNkd5vPsoJ/6a84fiaWp9pAnna9VGz0/DA4J3jNoByA1TuCg==
</details>
122 | + - Regex: '.*' 123 | + Priority: 1 124 | + SortPriority: 0 125 | + CaseSensitive: false 126 | +IncludeIsMainRegex: '(Test)?$' 127 | +IncludeIsMainSourceRegex: ''
unrelated note: I understand the setting IncludeBlocks is set to Preserve (and it should probably stay this way by default, but it would be nice to update the other Include* settings (in another pull request) to properly sort the includes: (1) the main include, (2) everything else (Bitcoin Core includes) (3) third-party library includes (boost, Qt), (4) stdlib includes (those that do not end in .h). This way, one could toggle the IncludeBlocks to get the desired behavior.
66 | @@ -67,7 +67,7 @@ Coding Style (C++) 67 | [src/.clang-format](/src/.clang-format). You can use the provided 68 | [clang-format-diff script](/contrib/devtools/README.md#clang-format-diffpy) 69 | tool to clean up patches automatically before submission. 70 | - - Braces on new lines for classes, functions, methods. 71 | + - Braces on new lines for classes, structs, functions, methods.
Why change the C++ Coding Style? I got used to the current one, which encourages structs for POD as a separate thing.
We can define it that way as well (though I don't agree with the distinction vs classes), but not specifying it directly results in different versions formatting structs differently, so we should define it explicitly
Ran clang-format -i -- **.cpp **.h **.hpp in src/ for both AfterStruct true & false, and compared diffs. Man was it close.
| true (94364a7d5403d9d480ebc065f8709c6dd21e543f) | false |
|---|---|
struct\n{ |
struct { |
| 85'849 lines | 85'659 lines |
Even though struct & class can be used interchangeably in C++, only with different defaults for public/private, I think we might as well benefit from the fact that we have 2 concepts and use them to communicate which types are POD (more C-like) or not. (C# includes both too but places limits on struct such as disabling support for inheritance and treating them as value types by default).
I took for granted that the omission of struct on this line in developer-notes.md was intentional. Comes from #4498, but conversation unfortunately doesn't touch on struct. Author of that PR copyrighted a C-only library one year earlier (https://github.com/bitcoin-core/secp256k1/blob/master/COPYING#L1). It also has opening curlies on same line as struct.
Happy to have explicitly defined, but my vote is for false and not changing developer-notes.md. - Dropping the second commit.
I would be fine with both as long as it's explicit. I still find the rationale that struct and class should be formatted similarly, it's harder for me to explain why struct is more like a for loop than a class...
85'849 lines
I have no idea what these numbers mean, I checked same-line opening braces with
% grep -E '^\s*\bstruct\b\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l
393
and quickly vibe coded a script that does newlines:
<details> <summary>vibe-Details</summary>
#!/bin/bash
echo "Analyzing struct brace placement in Bitcoin Core..."
# Count same-line braces
same_line=$(grep -E '^\s*struct\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l)
# Count next-line braces using awk
next_line=$(find src/ \( -name "*.cpp" -o -name "*.h" \) -exec awk '
/^[[:space:]]*struct[[:space:]]+[^;{]*$/ {
struct_line = NR
getline
if (/^[[:space:]]*\{/) {
count++
}
}
END { print count+0 }
' {} \; | awk '{sum+=$1} END {print sum}')
echo "Structs with same-line brace: $same_line"
echo "Structs with next-line brace: $next_line"
echo "Total: $((same_line + next_line))"
</details>
which vibe-revealed:
Analyzing struct brace placement in Bitcoin Core...
Structs with same-line brace: 393
Structs with next-line brace: 223
Total: 616
So the counts don't really matter in my opinion, we have a few hundred of each, there's no clear winner based on usage, let's come up with better arguments for either.
The line-counts where from the resulting git diffs. Admittedly they are imprecise due to surrounding context and other clang-format changes, but they give a rough relative estimate.
Thanks for vibing, almost double on same line. I'd say that does matter.
Edit:
it's harder for me to explain why
structis more like a for loop than aclass...
Maybe seeing struct as a small/passive/record thing and seeing class as more of an alive/process thing with multiple methods and internal logic makes it closer to a function? Main thing for me is to keep to what a majority of the current code and coding style says, otherwise this change should probably be discussed more broadly.
Admittedly they are imprecise
I'd say ~85000 vs ~400 is a bit more than imprecise :)
almost double on same line
That's not how we usually decide outcomes, rather by better explanations - especially since many of those aren't counting declarations, e.g
I'm fine with both, but have better explanations for the one I'm proposing. Ultimately I don't think it matters much, as long as it's explicit since current behavior results in inconsistencies.
Maybe seeing struct as a small/passive/record thing and seeing class as more of an alive/process
That sounds more philosophical to me, I don't think that's how we're using them (or how formatting is usually decided)? Most definitions I see state: it's just a class with public members, e.g. https://en.wikipedia.org/wiki/Struct_(C_programming_language)#C++
a class is the same as a struct but with different default visibility
Not sure why the formatting should be closer to an if or a switch or a loop, I don't see how it's closer to those that to a class.
Clang-format itself usually bundles the class and struct formatting together:
especially since many of those aren't counting declarations, e.g ...
That is precisely one of the reasons why running clang-format with the two different values on the entire src/ and comparing the relative diff size is helpful. Lines such as the ones you quoted will not contribute to the relative difference in the size of the change.
That's not how we usually decide outcomes, rather by better explanations -
I try to present explanations, but then you respond with:
That sounds more philosophical to me
But I've been brewing on a variation of the argument. Say you have a C coding style with struct Foo { and function\n{. Then you start introducing C++. Suddenly you have these struct-like classes but they can contain functions. C++ came from Bjarne's "C with classes" https://en.wikipedia.org/wiki/Cfront. class could contain functions and so allowing class Foo\n{ would seem more okay. structs could also (eventually?) contain functions since it was sometimes useful. I know this argument is somewhat blunt, but I think our current style is influenced by the historical progression of C++ coming out of C.
Elaborating on my libsecp256k1 argument since you did not respond to it:
sipa both added the section to the coding style and wrote that library to replace OpenSSL-usage. They both happened within a year of each other. Keeping styles consistent between the two closely related project is nice for developers working on both (silent payments, musig2 and batch signature verification comes to mind as projects which span both projects). So even if our struct style is understated, I would wager that your change to developer-notes.md constitutes a change of style rather than a clarification or specifying something that was totally arbitrary.
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.
Noting down further arguments for the current version of the PR:
AfterStruct and AfterClass were both introduced as part of LLVM 3.8.0 in 2016 (See https://releases.llvm.org/3.8.0/tools/clang/docs/ClangFormatStyleOptions.html vs https://releases.llvm.org/3.7.0/tools/clang/docs/ClangFormatStyleOptions.html), yet only the latter was added in to our .clang-format in cd0e1e91dd6d020263433d85526a91181180471d.clang-format doesn't infer AfterStruct from AfterClass when the former is unset.
ACK 7c9b3e1eae8f206753457149f1b1c837f6627d6d (first commit)
Ran clang-format -dump-config -style=file:src/.clang-format > foo on that commit and diffed foo against src/.clang-format. Only differences where added/elaborated settings from me running Clang-format 19. Makes sense to keep the format at the minimum version we support.
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.
Regenerated `.clang-format` from current configs to replace deprecated keys with up-to-date equivalents.
Also added all current formatter default values to guard against version differences.
The configs were updated with the following command (using v16 for maximal compatibility):
$(brew --prefix llvm@16)/bin/clang-format -dump-config -style=file:src/.clang-format
The new config was tested with:
$(brew --prefix llvm@16)/bin/clang-format -i src/deploymentinfo.h
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>172 | +PenaltyReturnTypeOnItsOwnLine: 60 173 | PointerAlignment: Left 174 | +PPIndentWidth: -1 175 | +QualifierAlignment: Leave 176 | +ReferenceAlignment: Pointer 177 | +ReflowComments: true
nit: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments
We might want to set IndentOnly, so we don't unnecessarily touch lines. But probably better to follow up on this in another PR, similar to initializer lists (https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258).
similar to initializer lists (#32740 (comment)).
When formatting initializer lists, one can manually put the : on the next line. This should give a stable clang-format output. (With the current line limit, clang-format won't split or merge lines by itself)
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31
struct brace style was made to explicitly differ from class in developer-notes.md after some back & forth (https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2303151367).
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31 🖼
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31 🖼
gVHv+GHN3tcL9TjuYLc/42PdtTtoJTNhIIFWKWk6RVQ/h7A3PizxMW48bbhzO4bbFkEQlpnI+nyNIkwIbsKHCg==
</details>
rfm
ACK 13f36c020f0329b5e975282b45292fdf2a495e31
