New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
style: How to format multi-line if condition with clang-format? #21735
Comments
agree that this is aesthetically misleading. to brainstorm, what do you think about this for when the conditionals are short enough:
and this for when they are longer:
|
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 One hack would be to use double if ((nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE)) {
BOOST_CHECK_EQUAL(min_activation_height, 0);
return;
} |
For multi-line if conditions I personally prefer this way, where I add newline after first if (
nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE ||
nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE
) {
BOOST_CHECK_EQUAL(min_activation_height, 0);
return;
} But have no idea how that works with clang-format. |
How about placing the boolean operator at the start of the new line:
This is recommended by the GNU coding standards (https://www.gnu.org/prep/standards/html_node/Formatting.html):
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 |
That could be set via |
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? if (short_condition) trivial_action();
if (short_condition) {
complex();
action();
}
if (one_complicated_condition
&& another_complicated_condition)
{
trivial_or_complex_action();
} Specifying |
Another hack would be to add if (nStartTime == Consensus::BIP9Deployment::ALWAYS_ACTIVE
|| nStartTime == Consensus::BIP9Deployment::NEVER_ACTIVE) //
{
BOOST_CHECK_EQUAL(min_activation_height, 0);
return;
} I couldn't get |
Isn't this discussion irrelevant with the current unlimited line length ( Otherwise, I like @jnewbery's suggestion above. |
No. I wasn't aware of this option. Thanks for suggesting. Fixed in #23216.
I can't reproduce. Does this happen with the example "dirty" diff in #23216 (comment) ? 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: // Example1
if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent
&& now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { is joined into a single line: 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: // Example2
if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent &&
now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { I guess this is due to
No, because it breaks after
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. |
Right, the column limit needs to be set first before looking into this. |
FWIW, I run a shell wrapper of clang-format that:
The unlimited line length creates too many problems for me, so I solve it locally. Have been afraid to raise the topic publicly 🔥 |
Same. This is the patch I use. (150 is also the limit on the GitHub web view) diff --git a/src/.clang-format b/src/.clang-format
index 791b3b8f9f..8da3baf34a 100644
--- a/src/.clang-format
+++ b/src/.clang-format
@@ -17,10 +17,11 @@ BreakBeforeBinaryOperators: false
BreakBeforeBraces: Custom
BraceWrapping:
AfterClass: true
+ AfterControlStatement: MultiLine
AfterFunction: true
BreakBeforeTernaryOperators: false
BreakConstructorInitializersBeforeComma: false
-ColumnLimit: 0
+ColumnLimit: 150
CommentPragmas: '^ IWYU pragma:'
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4 |
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 |
Ok, I meant the GitHub line limit with an unlimited monitor. Locally I get 153 chars for https://github.com/martinus/github-character-length/pull/2/files |
For me: Diff, unified view: 129 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 |
It shouldn't be important what the exact limit is, as it is only used to format touched code, not reformat the whole codebase. If there is a reason, it can always be adjusted. Though, initially it shouldn't be reduced too much, since it is currently unlimited. |
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:
The text was updated successfully, but these errors were encountered: