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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32813.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
44 SpacesInCStyleCastParentheses: false
45 SpacesInParentheses: false
46-BreakBeforeConceptDeclarations: Always
47-RequiresExpressionIndentation: OuterScope
48-Standard: c++20
49-UseTab: Never
Again, as said in my previous comment this doesn’t work, because older clang-formats will just error out:
0$ clang-format-16 -i src/bench/prevector.cpp
1/b-c/src/.clang-format:12:3: error: unknown key 'AlignFunctionPointers'
2 AlignFunctionPointers: false
3 ^~~~~~~~~~~~~~~~~~~~~
4Error reading /b-c/src/.clang-format: Invalid argument
0@@ -1,26 +1,40 @@
1 AccessModifierOffset: -4
2-AlignAfterOpenBracket: true
3+AlignAfterOpenBracket: Align
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44780552247
LLM reason (✨ experimental): The CI failure is caused by errors in the lint check, related to code style or formatting issues.
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.
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:
0# clang-format-16 --version
1Ubuntu clang-format version 16.0.6 (23ubuntu4)
However, the command:
0clang-format-16 -dump-config > .clang-format
produces a different result:
0# git diff -U0
1diff --git a/src/.clang-format b/src/.clang-format
2index 2f3f96ae2e..0822db5d11 100644
3--- a/src/.clang-format
4+++ b/src/.clang-format
5@@ -0,0 +1 @@
6+---
7@@ -2 +3,2 @@ Language: Cpp
8-AccessModifierOffset: -4
9+# BasedOnStyle: LLVM
10+AccessModifierOffset: -2
11@@ -29 +31 @@ AlignConsecutiveMacros:
12-AlignEscapedNewlines: Left
13+AlignEscapedNewlines: Right
14@@ -37 +39 @@ AllowShortBlocksOnASingleLine: Never
15-AllowShortCaseLabelsOnASingleLine: true
16+AllowShortCaseLabelsOnASingleLine: false
17@@ -40 +42 @@ AllowShortFunctionsOnASingleLine: All
18-AllowShortIfStatementsOnASingleLine: WithoutElse
19+AllowShortIfStatementsOnASingleLine: Never
20@@ -46 +48 @@ AlwaysBreakBeforeMultilineStrings: false
21-AlwaysBreakTemplateDeclarations: Yes
22+AlwaysBreakTemplateDeclarations: MultiLine
23@@ -54 +56 @@ BraceWrapping:
24- AfterClass: true
25+ AfterClass: false
26@@ -58 +60 @@ BraceWrapping:
27- AfterFunction: true
28+ AfterFunction: false
29@@ -61 +63 @@ BraceWrapping:
30- AfterStruct: true
31+ AfterStruct: false
32@@ -76 +78 @@ BreakBeforeConceptDeclarations: Always
33-BreakBeforeBraces: Custom
34+BreakBeforeBraces: Attach
35@@ -78 +80 @@ BreakBeforeInlineASMColon: OnlyMultiline
36-BreakBeforeTernaryOperators: false
37+BreakBeforeTernaryOperators: true
38@@ -82 +84 @@ BreakStringLiterals: true
39-ColumnLimit: 0
40+ColumnLimit: 80
41@@ -123 +125 @@ IndentRequiresClause: true
42-IndentWidth: 4
43+IndentWidth: 2
44@@ -137 +139 @@ JavaScriptWrapImports: true
45-KeepEmptyLinesAtTheStartOfBlocks: false
46+KeepEmptyLinesAtTheStartOfBlocks: true
47@@ -142 +144 @@ MacroBlockEnd: ''
48-MaxEmptyLinesToKeep: 2
49+MaxEmptyLinesToKeep: 1
50@@ -160 +162 @@ PenaltyReturnTypeOnItsOwnLine: 60
51-PointerAlignment: Left
52+PointerAlignment: Right
53@@ -208 +210 @@ SpacesInSquareBrackets: false
54-Standard: c++20
55+Standard: Latest
56@@ -222,0 +225 @@ WhitespaceSensitiveMacros:
57+
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).
0$ clang-format-16 --version
1Ubuntu clang-format version 16.0.6 (23ubuntu4)
2$ clang-format-16 -dump-config | grep AfterClass
3 AfterClass: false
4$ clang-format-16 -dump-config -style=file:src/.clang-format | grep AfterClass
5 AfterClass: true
and
0$ 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'
1AccessModifierOffset: -4
2AlignEscapedNewlines: Left
3AllowShortCaseLabelsOnASingleLine: true
4AllowShortIfStatementsOnASingleLine: WithoutElse
5AlwaysBreakTemplateDeclarations: Yes
6 AfterClass: true
7 AfterFunction: true
8 AfterStruct: false
9BreakBeforeBraces: Custom
10BreakBeforeTernaryOperators: false
11ColumnLimit: 0
12ConstructorInitializerIndentWidth: 4
13ContinuationIndentWidth: 4
14DerivePointerAlignment: false
15IndentWidth: 4
16KeepEmptyLinesAtTheStartOfBlocks: false
17MaxEmptyLinesToKeep: 2
18ObjCBlockIndentWidth: 2
19PointerAlignment: Left
20PPIndentWidth: -1
21 AfterFunctionDefinitionName: false
22 AfterFunctionDeclarationName: false
23Standard: 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 🥙
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 94364a7d5403d9d480ebc065f8709c6dd21e543f 🥙
3xsnBRKa1+85+2JOjTRQ1kZgSlBHhVZxj7LlQpMQNkd5vPsoJ/6a84fiaWp9pAnna9VGz0/DA4J3jNoByA1TuCg==
122+ - Regex: '.*'
123+ Priority: 1
124+ SortPriority: 0
125+ CaseSensitive: false
126+IncludeIsMainRegex: '(Test)?$'
127+IncludeIsMainSourceRegex: ''
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.
structs for POD as a separate thing.
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
0% grep -E '^\s*\bstruct\b\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l
1 393
and quickly vibe coded a script that does newlines:
0#!/bin/bash
1
2echo "Analyzing struct brace placement in Bitcoin Core..."
3
4# Count same-line braces
5same_line=$(grep -E '^\s*struct\s+[^;{]*\{' --include="*.h" --include="*.cpp" -r src/ | wc -l)
6
7# Count next-line braces using awk
8next_line=$(find src/ \( -name "*.cpp" -o -name "*.h" \) -exec awk '
9/^[[:space:]]*struct[[:space:]]+[^;{]*$/ {
10 struct_line = NR
11 getline
12 if (/^[[:space:]]*\{/) {
13 count++
14 }
15}
16END { print count+0 }
17' {} \; | awk '{sum+=$1} END {print sum}')
18
19echo "Structs with same-line brace: $same_line"
20echo "Structs with next-line brace: $next_line"
21echo "Total: $((same_line + next_line))"
which vibe-revealed:
0Analyzing struct brace placement in Bitcoin Core...
1Structs with same-line brace: 393
2Structs with next-line brace: 223
3Total: 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.
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.
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 🖼
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31 🖼
3gVHv+GHN3tcL9TjuYLc/42PdtTtoJTNhIIFWKWk6RVQ/h7A3PizxMW48bbhzO4bbFkEQlpnI+nyNIkwIbsKHCg==
