Usually clang-format is overall producing a nice formatting style. However, it doesn’t handle multi-line if conditions well.
Currently it will put the condition as well as the true-block on the same indentation. Making them appear to be one block. This isn’t nice to the eye and I wouldn’t be surprised if it lead to bugs eventually.
Example:
0 if (IsThisTrue() ||
1 IsThatTrue()) {
2 DoThis;
3 DoThat;
4 }
agree that this is aesthetically misleading. to brainstorm,
what do you think about this for when the conditionals are short enough:
0if (IsThisTrue() || IsThatTrue()) {
1 DoThis;
2 DoThat;
3 }
and this for when they are longer:
0if (IsThisTrue()
1 || IsThatTrue()) {
2 DoThis;
3 DoThat;
4 }
Yeah, I think single-line if-conditions are already fine. And for multi-line conditions I am looking for something that works with clang-format, ideally. Right now I manually put the opening { on a new line, which will be undone the next time I call clang-format.
One hack would be to use double (()) around the condition.
0 if ((nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
1 nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE)) {
2 BOOST_CHECK_EQUAL(min_activation_height, 0);
3 return;
4 }
For multi-line if conditions I personally prefer this way, where I add newline after first ( and before last ), so that it’s better division between last line of condition and first line of code in if block.
0 if (
1 nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
2 nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE
3 ) {
4 BOOST_CHECK_EQUAL(min_activation_height, 0);
5 return;
6 }
But have no idea how that works with clang-format.
How about placing the boolean operator at the start of the new line:
0 if (IsThisTrue()
1 || IsThatTrue()) {
2 DoThis;
3 DoThat;
4 }
This is recommended by the GNU coding standards (https://www.gnu.org/prep/standards/html_node/Formatting.html):
When you split an expression into multiple lines, split it before an operator, not after one. Here is the right way:
0if (foo_this_is_long && bar > win (x, y, z) 1 && remaining_condition)Try to avoid having two operators of different precedence at the same level of indentation. For example, don’t write this:
0mode = (inmode[j] == VOIDmode 1 || GET_MODE_SIZE (outmode[j]) > GET_MODE_SIZE (inmode[j]) 2 ? outmode[j] : inmode[j]);Instead, use extra parentheses so that the indentation shows the nesting:
0mode = ((inmode[j] == VOIDmode 1 || (GET_MODE_SIZE (outmode[j]) > GET_MODE_SIZE (inmode[j]))) 2 ? outmode[j] : inmode[j]);[…]
Google C++ standards just say “When you have a boolean expression that is longer than the standard line length, be consistent in how you break up the lines.” (https://google.github.io/styleguide/cppguide.html#Boolean_Expressions)
A bit more discussion here: https://news.ycombinator.com/item?id=7975386
BreakBeforeBinaryOperators in clang-format
This is recommended by the GNU coding standards (https://www.gnu.org/prep/standards/html_node/Formatting.html):
Agree (after first attempting to write things in a way that avoids the need for multi-line conditionals).
Has open-brace-on-new-line already been ruled out for multi-line conditions?
0if (short_condition) trivial_action();
1
2if (short_condition) {
3 complex();
4 action();
5}
6
7if (one_complicated_condition
8 && another_complicated_condition)
9{
10 trivial_or_complex_action();
11}
Specifying BraceWrapping: AfterControlStatement: MultiLine stops clang-format from undoing that style, I think – but we’d need to set ColumnLimit: to something for it to actually stop you from putting an open brace at the end of a multiline if.
Right now I manually put the opening { on a new line, which will be undone the next time I call clang-format.
Another hack would be to add // at the end of the if. In combination with BreakBeforeBinaryOperators: NonAssignment clang-format wouldn’t change this any more:
0if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE
1 || nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) //
2{
3 BOOST_CHECK_EQUAL(min_activation_height, 0);
4 return;
5}
I couldn’t get BraceWrapping: AfterControlStatement working with that.
Isn’t this discussion irrelevant with the current unlimited line length (ColumnLimit: 0)? For me, no matter how I break the line manually, clang-format joins all the lines into a single line (unlimitedly long one). Also it never decides to break a long line.
Otherwise, I like @jnewbery’s suggestion above. BreakBeforeBinaryOperators: true does just that.
Has open-brace-on-new-line already been ruled out for multi-line conditions?
No. I wasn’t aware of this option. Thanks for suggesting. Fixed in #23216.
For me, no matter how I break the line manually, clang-format joins all the lines into a single line (unlimitedly long one).
I can’t reproduce. Does this happen with the example “dirty” diff in #23216#issue-753033469 ? Which version of clang-format are you using?
I played a bit with this. It looks like clang-format would join lines only sometimes. Not always, as I was thinking before. For example, with the current config, the following:
0 // Example1
1 if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent
2 && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
is joined into a single line:
0 if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
But this remains unchanged:
0 // Example2
1 if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent &&
2 now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
I guess this is due to BreakBeforeBinaryOperators: false. If I change that to true, then Example1 remains unchanged while Example2 is joined into a single line.
Does this happen with the example “dirty” diff…
No, because it breaks after &&.
Which version of clang-format are you using?
Tried with 12 and 14.
It looks like manually inserted line breaks, if they do not violate some rules, are respected and not removed. If they violate e.g. BreakBeforeBinaryOperators then they are removed and lines joined.
Specifying BraceWrapping: AfterControlStatement: MultiLine stops clang-format from undoing that style, I think – but we’d need to set ColumnLimit: to something for it to actually stop you from putting an open brace at the end of a multiline if.
Isn’t this discussion irrelevant with the current unlimited line length (ColumnLimit: 0)?
Right, the column limit needs to be set first before looking into this.
FWIW, I run a shell wrapper of clang-format that:
ColumnLimit from 0 to 100 in src/.clang-formatsrc/.clang-format (so that I don’t commit it accidentally)The unlimited line length creates too many problems for me, so I solve it locally. Have been afraid to raise the topic publicly :fire:
FWIW, I run a shell wrapper of clang-format that:
Same.
This is the patch I use. (150 is also the limit on the GitHub web view)
0diff --git a/src/.clang-format b/src/.clang-format
1index 791b3b8f9f..8da3baf34a 100644
2--- a/src/.clang-format
3+++ b/src/.clang-format
4@@ -17,10 +17,11 @@ BreakBeforeBinaryOperators: false
5 BreakBeforeBraces: Custom
6 BraceWrapping:
7 AfterClass: true
8+ AfterControlStatement: MultiLine
9 AfterFunction: true
10 BreakBeforeTernaryOperators: false
11 BreakConstructorInitializersBeforeComma: false
12-ColumnLimit: 0
13+ColumnLimit: 150
14 CommentPragmas: '^ IWYU pragma:'
15 ConstructorInitializerAllOnOneLineOrOnePerLine: false
16 ConstructorInitializerIndentWidth: 4
150 is also the limit on the GitHub web view
I’m pretty sure that GitHub’s limit depends on your monitor and fonts used. For me a diff on github can have 156 characters. I’ve created a little project to see the line lengths: https://github.com/martinus/github-character-length
I would very much appreciate it if we could add a reasonable default ColumnLimit to the .clang-format. I’m fine with anything in the range 80-150. I usually use around 130 or so.
For me:
Diff, unified view: 129 Diff, split view: 62 Comment in PR: 80
but yes, it quickly changes if I zoom in/out (ctrl + +/-). I think Github’s sh**y web UI, that could as well change, shouldn’t be a deciding factor for ColumnLimit in .clang-format.