Omitting curly braces from short if statements: Ban or not?

[K. Moon]

Picking up on the discussion that started on the #cxx Slack, with regards to the latest style guide update, which now says:

For historical reasons, we allow one exception to the above rules: if an if statement has no else or else if clauses, then the curly braces for the controlled statement or the line breaks inside the curly braces may be omitted if as a result the entire if statement appears on either a single line (in which case there is a space between the closing parenthesis and the controlled statement) or on two lines (in which case there is a line break after the closing parenthesis and there are no braces). For example, the following forms are allowed under this exception.
if (x == kFoo) return new Foo();
if (x == kBar)
  return new Bar(arg1, arg2, arg3);
if (x == kQuz) { return new Quz(1, 2, 3); }
Use this style only when the statement is brief, and consider that conditional statements with complex conditions or controlled statements may be more readable with curly braces. Some projects require curly braces always.

(I've underlined the most pertinent parts.)

This has led to debate on the following points:

1. Does the "historical reasons" clause imply that the braceless style should be discouraged in new code?
2. Should Chromium, as a project, take a stance on whether to always require braces? (If required, this would be enforced with tooling.)

I think there are good arguments on both sides, but I'm going to avoid injecting my personal opinion in this first post. :-)

[Peter Kasting]

Re 1: At least on #cxx on Slack, it seemed like we reached some level of consensus that braces are not discouraged in new code.  My next point assumes this interpretation.

Re 2: Classically, we've tried to avoid going beyond the Google Style Guide unless there is a Chromium-specific reason to do so.  Do we have Chromium-specific reasons to take a stronger stance against braceless conditionals than the Google style guide does?

PK

[Nico Weber]

+1 to pkasting. Doesn't seem worth making rules around.

I'm point out that we build with both -Wmisleading-indentation and -Wunreachable-code which both find problems like

  if (foo)

    a();

    b();

at compile time, so the no-brace ifs shouldn't cause many bugs.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAfW%2B9xq3noJKb99bGF-Xzr6_W6hTvipJ_1HfPVa8fj%2Bg%40mail.gmail.com.

[Daniel Cheng]

In the past, I've had review comments asking me to remove braces. Would it be reasonable to say that this is explicitly allowed going forward?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiH_AvqM2QFGcg2VeDQVhA-xK8NZbiZb%2BbPAvsU2_F5QJg%40mail.gmail.com.

[Peter Kasting]

On Wed, Nov 11, 2020 at 10:27 AM Daniel Cheng <dch...@chromium.org> wrote:

In the past, I've had review comments asking me to remove braces. Would it be reasonable to say that this is explicitly allowed going forward?

I don't know that such a statement pulls its weight in the Chromium style guide, but I certainly think it would be reasonable if that's the conclusion of this thread to push back and reference this thread.

PK 

[dan...@chromium.org]

I would like to see reviewers not require braces to be added or removed from CLs that do not violate the style guide.

And I do agree that the guide explicitly allows excluding them for 2-line `if+controled statement` situations. This is more restricted than in the past, but it's clear that it allows simple early-outs etc to stay without braces. It also allows them to have braces.

 

PK 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDhkZ2%3DgULEDMXDFAjnZVjKPdeQkmu49%2BXncVTsLuo6og%40mail.gmail.com.

[Joe Mason]

We should have "git cl format" adjust braces to whatever the style guide says. If I'm not mistaken it currently doesn't touch braces because it's hard to get the rules right, but requiring braces around conditionals would be easy to implement. So we should do that.  (Plus it would be weird to have tooling prefer the optional format that's allowed but not encouraged.)

Whether that means "the style guide says braces are always required" or "the style guide says braces are always allowed; in theory you can choose not to use them for one-liners but in practice git cl format will add them back" is just semantics.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQmf6nChwmw51ieJ3e9qZc44Sm4SBr0XbABtCfH8XB5Ww%40mail.gmail.com.

[Ken Rockot]

On Wed, Nov 11, 2020 at 10:45 AM 'Joe Mason' via cxx <c...@chromium.org> wrote:

We should have "git cl format" adjust braces to whatever the style guide says. If I'm not mistaken it currently doesn't touch braces because it's hard to get the rules right, but requiring braces around conditionals would be easy to implement. So we should do that.  (Plus it would be weird to have tooling prefer the optional format that's allowed but not encouraged.)

Whether that means "the style guide says braces are always required" or "the style guide says braces are always allowed; in theory you can choose not to use them for one-liners but in practice git cl format will add them back" is just semantics.

I really don't want clang-format adding braces to my one-line conditionals. It should be possible for both braced and braceless one-liners to be preserved as-is.

 

On Wed, Nov 11, 2020 at 1:36 PM <dan...@chromium.org> wrote:

On Wed, Nov 11, 2020 at 1:29 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Nov 11, 2020 at 10:27 AM Daniel Cheng <dch...@chromium.org> wrote:

In the past, I've had review comments asking me to remove braces. Would it be reasonable to say that this is explicitly allowed going forward?

I don't know that such a statement pulls its weight in the Chromium style guide, but I certainly think it would be reasonable if that's the conclusion of this thread to push back and reference this thread.

I would like to see reviewers not require braces to be added or removed from CLs that do not violate the style guide.

And I do agree that the guide explicitly allows excluding them for 2-line `if+controled statement` situations. This is more restricted than in the past, but it's clear that it allows simple early-outs etc to stay without braces. It also allows them to have braces.

 

PK 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDhkZ2%3DgULEDMXDFAjnZVjKPdeQkmu49%2BXncVTsLuo6og%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQmf6nChwmw51ieJ3e9qZc44Sm4SBr0XbABtCfH8XB5Ww%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95RDYX8XiuRGM5WkFwd95p-Ca%2Btt6TieyRnDSRVrmqO%3DKw%40mail.gmail.com.

[Peter Kasting]

On Wed, Nov 11, 2020 at 10:45 AM Joe Mason <joenot...@google.com> wrote:

We should have "git cl format" adjust braces to whatever the style guide says. If I'm not mistaken it currently doesn't touch braces because it's hard to get the rules right,

We already have a global .clang-tidy config that, basically, does exactly this, including allowing no-braces on single-line conditionals (at least for some set of those).

PK

[Peter Kasting]

Retraction: The above statement is false.

We should add "readability-braces-around-statements" with "ShortStatementLines" configured to 1 in the global .clang-tidy in order to get this.  (I thought this was already true; it is not.)

PK

[dan...@chromium.org]

From https://releases.llvm.org/8.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-braces-around-statements.html it looks like we need further configurability to disallow one-line-statements when an else is present.

 

PK

[Peter Kasting]

Yes, this will still allow braces in some cases that shouldn't be; it will catch the majority of problems.  I'm sure "patches welcome" is true given that the global Google styleguide changed.

PK 

[Jeremy Roman]

Since this style:

if (foo)

  bar();

is both common in Chromium and permitted by the style guide, I don't see any reason to ban it.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQ5HYPQJBHeZnDuDJVaMg3OwP_DqgkdY9q-Br7k2Vnz7A%40mail.gmail.com.

[Tal Pressman]

While I do have a slight preference for always requiring braces, I'll echo what's been said above - As long as this is enforced/fixed by tooling, and we don't have to worry about it in code reviews, either way SGTM.

Tal

[Aleks Totic]

Are braces around single line statements encouraged? 

I prefer them because then I never have to worry about code reformat after rename splitting one line into two, and enforcing braces on upload.

But, my single line braces often get taken out in a code review. If they are encouraged, that'd be good to know.

Aleks 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2a6cbe79-d454-4ca3-890a-a21bf0b6bc91n%40chromium.org.

[Nico Weber]

The guidance in the text cited in post 0 on this thread is "if an if statement has no else or else if clauses, then the curly braces for the controlled statement or the line breaks inside the curly braces may be omitted if as a result the entire if statement appears on either a single line", and consensus on this thread seems to be to keep this as-is.

So people shouldn't bikeshed about braces where either style is allowed per style guide in reviews.

[K. Moon]

To summarize a bit where we are, I think we have consensus on the following points:

1. The style guide allows omitting braces (in certain cases), and the update doesn't change that.
2. We shouldn't care about this if tooling can handle the issue.
3. Since the style guide permits both, code reviewers shouldn't mandate one style or another.

I don't think this settles the question, though, as I think these points still leave plenty unresolved:

1. The style guide allows omitting braces, but does it recommend it? IMO, the language is unclear on this, and to me, the situation feels a bit how MACRO_CASE used to be allowed for enumerators, but is now banned completely: Too many people are invested in the current way of doing things, but it's not the ideal rule.
2. It's been mentioned that clang-tidy can enforce allowing both styles, but (1) clang-tidy doesn't run as part of git cl format, so it has a higher cost to apply (I've often gone back and fixed issues only caught by clang-tidy after I've uploaded a change), and (2) since both styles are allowed, tools still can't make decisions about whether or not to include the braces, only that they're included or omitted correctly. Human judgment is still required in each case.
3. Reviewers can still use consistency and subjective readability to bikeshed about whether or not to include braces. (If nobody thought the style without braces wasn't more readable, we wouldn't be discussing this, after all.) A number of people have mentioned having the experience that they've included the braces, and been told to remove them. (It seems rarer to get review comments in the other direction, which makes me think people feel more strongly about omitting them than including them.)

I'd like to make two additional arguments:

First, Nico raised how -Wmisleading-indentation and -Wunreachable-code would catch something like this, so we don't need to worry about it:

  if (foo)
    a();
    b();

I hadn't even considered this, but I realized this is actually a technical argument for preferring always using braces. For one thing, these compile-time checks are not going to catch the common scenario where you might (1) do a search-and-replace, and then (2) run clang-format ("fixing" the "misleading" indentation; oops). (There's also the additional toil of having to go in and add the braces where they weren't before. If the changes were temporary, you then have to remove them again...)

Second, while experienced developers already used to omitting the braces may feel more comfortable with that style, I think it's more important to think about how new developers are going to feel about it. (There will always be more future developers than current developers, after all, at least on a healthy project.) Having a single consistent rule here (either "always omit whenever possible," or "always include") is going to be easier to learn than something that requires experience and judgment. (And I would argue "always include" is the easier rule to follow, but I realize that's subjective.)

Finally, can someone state the arguments in favor of omitting braces? I feel like most of the arguments so far have been, "If the style guide allows it, why not allow it?" I think the case would be stronger with more affirmative arguments, though, such as, "If this were banned, my code would suffer because reason X." I'm assuming it's based on finding the more compact style more readable (early return in guards seems like a frequent example), just like we still have an 80 column limit, but I'm just guessing what the arguments are here. :-)

[dan...@chromium.org]

On Thu, Nov 12, 2020 at 11:59 AM K. Moon <km...@chromium.org> wrote:

To summarize a bit where we are, I think we have consensus on the following points:

1. The style guide allows omitting braces (in certain cases), and the update doesn't change that.
2. We shouldn't care about this if tooling can handle the issue.
3. Since the style guide permits both, code reviewers shouldn't mandate one style or another.

I don't think this settles the question, though, as I think these points still leave plenty unresolved:

1. The style guide allows omitting braces, but does it recommend it? IMO, the language is unclear on this, and to me, the situation feels a bit how MACRO_CASE used to be allowed for enumerators, but is now banned completely: Too many people are invested in the current way of doing things, but it's not the ideal rule.

I don't read it as recommending either way. It simply says what are valid styles here. And it explains why braceless style is valid, as because of the overwhelming number of people/code that use it.

1. It's been mentioned that clang-tidy can enforce allowing both styles, but (1) clang-tidy doesn't run as part of git cl format, so it has a higher cost to apply (I've often gone back and fixed issues only caught by clang-tidy after I've uploaded a change), and (2) since both styles are allowed, tools still can't make decisions about whether or not to include the braces, only that they're included or omitted correctly. Human judgment is still required in each case.
2. Reviewers can still use consistency and subjective readability to bikeshed about whether or not to include braces. (If nobody thought the style without braces wasn't more readable, we wouldn't be discussing this, after all.) A number of people have mentioned having the experience that they've included the braces, and been told to remove them. (It seems rarer to get review comments in the other direction, which makes me think people feel more strongly about omitting them than including them.)

I'd like to make two additional arguments:

First, Nico raised how -Wmisleading-indentation and -Wunreachable-code would catch something like this, so we don't need to worry about it:

  if (foo)
    a();
    b();

I hadn't even considered this, but I realized this is actually a technical argument for preferring always using braces. For one thing, these compile-time checks are not going to catch the common scenario where you might (1) do a search-and-replace, and then (2) run clang-format ("fixing" the "misleading" indentation; oops). (There's also the additional toil of having to go in and add the braces where they weren't before. If the changes were temporary, you then have to remove them again...)

Second, while experienced developers already used to omitting the braces may feel more comfortable with that style, I think it's more important to think about how new developers are going to feel about it. (There will always be more future developers than current developers, after all, at least on a healthy project.) Having a single consistent rule here (either "always omit whenever possible," or "always include") is going to be easier to learn than something that requires experience and judgment. (And I would argue "always include" is the easier rule to follow, but I realize that's subjective.)

Finally, can someone state the arguments in favor of omitting braces? I feel like most of the arguments so far have been, "If the style guide allows it, why not allow it?" I think the case would be stronger with more affirmative arguments, though, such as, "If this were banned, my code would suffer because reason X." I'm assuming it's based on finding the more compact style more readable (early return in guards seems like a frequent example), just like we still have an 80 column limit, but I'm just guessing what the arguments are here. :-)

AFAIK the benefit is to avoid the "noise" of {} in a short if statement, and the extra line of code that is required as a result. Though often an if () return is followed by a blank line already.

[Peter Kasting]

On Thu, Nov 12, 2020 at 8:59 AM K. Moon <km...@chromium.org> wrote:

Finally, can someone state the arguments in favor of omitting braces? I feel like most of the arguments so far have been, "If the style guide allows it, why not allow it?" I think the case would be stronger with more affirmative arguments, though, such as, "If this were banned, my code would suffer because reason X." I'm assuming it's based on finding the more compact style more readable (early return in guards seems like a frequent example), just like we still have an 80 column limit, but I'm just guessing what the arguments are here. :-)

Since they're basically syntax rather than semantics, braces are noisy, leading to increased cognitive overhead.  The noise ratio increases as the conditional is shorter, so it's precisely on short conditionals where it's most valuable to omit them.

IOW, yes, readability/scannability.

As to the rest of your arguments -- since they're largely not specific to Chromium, I don't think they address the argument that our style guide should not go beyond the Google guide without a Chromium-specific reason to do so (which seems to be a fairly high-consensus, established-practice rule at this point).  So to me, they could be amazing arguments that we should definitely use braces, and it wouldn't mean I'd agree we should change our guide.

Note: the bit about clang-tidy is arguably Chromium-specific.  That said, clang-tidy runs on Gerrit, so if something is part of the clang-tidy rules, it will get flagged, just not necessarily pre-upload.

PK

[Ryan Sleevi]

I'm not sure; it seems like your two additional arguments are wanting to discuss whether or not we have consensus that the style guide permits both and code reviewers shouldn't mandate one style or the other.

Consider the "string switch" statement; that is, comparing one string for several possible values.

With braces, and "else after return" (which we removed the style guide discouragement), this is

if (foo == "a") {

  return "1 thing";
} else if (foo == "b") {

  return "2 thing";

} else if (foo == "c") {
  return "3 thing";

}

return "oops"

With braces, but no else after return, this expands further:

if (foo == "a") {

  return "1 thing";
}

if (foo == "b") {

  return "2 thing";

}

if (foo == "c") {
  return "3 thing";

}

return "oops"

with no braces, and no else after return (and assuming long-enough lines), this is

if (foo == "a")

  return "1 thing";
if (foo == "b")

  return "2 thing";

if (foo == "c")
  return "3 thing";

return "oops"

Effectively, the braces add vertical space in a way that may not help readability.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-711weLx%2BHJdZKJuARV3OsXpXyTGK5B31ZmhyiH4FD95w%40mail.gmail.com.

[Vladimir Levin]

On Thu, Nov 12, 2020 at 11:59 AM K. Moon <km...@chromium.org> wrote:

To summarize a bit where we are, I think we have consensus on the following points:

1. The style guide allows omitting braces (in certain cases), and the update doesn't change that.
2. We shouldn't care about this if tooling can handle the issue.
3. Since the style guide permits both, code reviewers shouldn't mandate one style or another.

I don't think this settles the question, though, as I think these points still leave plenty unresolved:

1. The style guide allows omitting braces, but does it recommend it? IMO, the language is unclear on this, and to me, the situation feels a bit how MACRO_CASE used to be allowed for enumerators, but is now banned completely: Too many people are invested in the current way of doing things, but it's not the ideal rule.
2. It's been mentioned that clang-tidy can enforce allowing both styles, but (1) clang-tidy doesn't run as part of git cl format, so it has a higher cost to apply (I've often gone back and fixed issues only caught by clang-tidy after I've uploaded a change), and (2) since both styles are allowed, tools still can't make decisions about whether or not to include the braces, only that they're included or omitted correctly. Human judgment is still required in each case.
3. Reviewers can still use consistency and subjective readability to bikeshed about whether or not to include braces. (If nobody thought the style without braces wasn't more readable, we wouldn't be discussing this, after all.) A number of people have mentioned having the experience that they've included the braces, and been told to remove them. (It seems rarer to get review comments in the other direction, which makes me think people feel more strongly about omitting them than including them.)

I'd like to make two additional arguments:

First, Nico raised how -Wmisleading-indentation and -Wunreachable-code would catch something like this, so we don't need to worry about it:

  if (foo)
    a();
    b();

I hadn't even considered this, but I realized this is actually a technical argument for preferring always using braces. For one thing, these compile-time checks are not going to catch the common scenario where you might (1) do a search-and-replace, and then (2) run clang-format ("fixing" the "misleading" indentation; oops). (There's also the additional toil of having to go in and add the braces where they weren't before. If the changes were temporary, you then have to remove them again...)

Second, while experienced developers already used to omitting the braces may feel more comfortable with that style, I think it's more important to think about how new developers are going to feel about it. (There will always be more future developers than current developers, after all, at least on a healthy project.) Having a single consistent rule here (either "always omit whenever possible," or "always include") is going to be easier to learn than something that requires experience and judgment. (And I would argue "always include" is the easier rule to follow, but I realize that's subjective.)

Finally, can someone state the arguments in favor of omitting braces? I feel like most of the arguments so far have been, "If the style guide allows it, why not allow it?" I think the case would be stronger with more affirmative arguments, though, such as, "If this were banned, my code would suffer because reason X." I'm assuming it's based on finding the more compact style more readable (early return in guards seems like a frequent example), just like we still have an 80 column limit, but I'm just guessing what the arguments are here. :-)

I think verbosity would be my main argument here. I don't particularly mind braces, but saying

if (!client)

  return;

is clear enough for me.

if (!client) {

  return;

}

says the same thing but with more characters. I don't feel like I've gained any particular safety / readability. It is nice that if I want to stick a printf in there, I don't have to add braces, but other than that it's more verbose than necessary (IMHO).

Same with

if (ShouldReturnFoo())

  return "foo";

if (ShouldReturnBar())

  return "bar";

I don't see adding braces here as an improvement:

if (ShouldReturnFoo()) {

  return "foo";

}

if (ShouldReturnBar()) {

  return "bar";

}

 It if the rule, then so be it, but if both are allowed, I would ask the reviewer to remove the braces (as a nit, of course)

Thanks!

Vlad

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-711weLx%2BHJdZKJuARV3OsXpXyTGK5B31ZmhyiH4FD95w%40mail.gmail.com.

[Vladimir Levin]

s/reviewer/author/

[Igor Bukanov]

On Thursday, November 12, 2020 at 6:10:13 PM UTC+1 Vladimir Levin wrote:

> I think verbosity would be my main argument here. I don't particularly mind braces, but saying

>

> if (!client)

>  return;

>

> is clear enough for me

This argument suggest to treat "if (condition) control_transfer;" differently from the "if" followed by non-jumpy code. Perhaps the rule should be that for the control transfer case there should be no braces unless "if (condition)" or the transfer like "return long_expression" does not fit one line. Or at least to always require braces only for non-jumpy code but allow not to use braces for jumps.

[Nico Weber]

Like Peter said, any argument here that's not chromium-specific in some way violates the "we're trying to follow the google style guide" default.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/d7211e63-b1b6-4f0f-86cb-8c1c28f05425n%40chromium.org.

[K. Moon]

I think this decision is a bit different than other possible deviations from the style guide, in that the style guide explicitly states that projects can make a choice. "Some projects require curly braces always," is an extremely unusual statement to see in the style guide, which generally doesn't suggest project-specific carve-outs. It also suggests opting out of the historical rule, but not vice versa.

That said, unless there's a sudden outpouring of support to the contrary, I don't think there's consensus for picking a single style on this, so let's consider that question decided.

I think there's still two more decisions to make in this thread:

1. What should tooling do?
2. How should reviewers handle disagreements about whether or not to omit braces?

With regards to (1), it sounds like we'd like to configure clang-tidy to (mostly) enforce the style guide rule, which SGTM.

With regards to (2), overall opinion seems to be leaning towards making this almost a "no comment" issue (whichever style-compliant choice the author makes should be respected), but I believe there have been at least one or two comments in this thread about preferring to omit in certain cases, which I think (unfairly?) privileges the historical option over the non-historical option.

[Nico Weber]

On Fri, Nov 13, 2020 at 1:41 PM K. Moon <km...@chromium.org> wrote:

I think this decision is a bit different than other possible deviations from the style guide, in that the style guide explicitly states that projects can make a choice. "Some projects require curly braces always," is an extremely unusual statement to see in the style guide, which generally doesn't suggest project-specific carve-outs. It also suggests opting out of the historical rule, but not vice versa.

That said, unless there's a sudden outpouring of support to the contrary, I don't think there's consensus for picking a single style on this, so let's consider that question decided.

I think there's still two more decisions to make in this thread:

1. What should tooling do?

If someone has bandwidth to try and implement something in clang-format, then they should implement something in clang-format :) IIRC adding this was harder than it at first seemed though. 

1. How should reviewers handle disagreements about whether or not to omit braces?

With regards to (1), it sounds like we'd like to configure clang-tidy to (mostly) enforce the style guide rule, which SGTM.

It's imho not a great use of time for reviewers to argue about this. The goal of code review is to make code better, and reviewers should try to ask themselves if they're adding value worth the back-and-forth overhead.

Personally I'd even say it's not worth it to make clang-tidy complain about it; if we add too many chatty diags to clang-tidy people will learn to ignore tricium comments. But I'm willing to have my mind changed on this point. https://chromium.googlesource.com/chromium/src/+/master/docs/clang_tidy.md#Adding-a-new-check describes the process for adding checks to clang-tidy, if you want to give it a try.

 

With regards to (2), overall opinion seems to be leaning towards making this almost a "no comment" issue (whichever style-compliant choice the author makes should be respected), but I believe there have been at least one or two comments in this thread about preferring to omit in certain cases, which I think (unfairly?) privileges the historical option over the non-historical option.

On Fri, Nov 13, 2020 at 7:06 AM Nico Weber <tha...@chromium.org> wrote:

Like Peter said, any argument here that's not chromium-specific in some way violates the "we're trying to follow the google style guide" default.

On Fri, Nov 13, 2020 at 10:02 AM Igor Bukanov <ig...@vivaldi.com> wrote:

On Thursday, November 12, 2020 at 6:10:13 PM UTC+1 Vladimir Levin wrote:

> I think verbosity would be my main argument here. I don't particularly mind braces, but saying

>

> if (!client)

>  return;

>

> is clear enough for me

This argument suggest to treat "if (condition) control_transfer;" differently from the "if" followed by non-jumpy code. Perhaps the rule should be that for the control transfer case there should be no braces unless "if (condition)" or the transfer like "return long_expression" does not fit one line. Or at least to always require braces only for non-jumpy code but allow not to use braces for jumps.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/d7211e63-b1b6-4f0f-86cb-8c1c28f05425n%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5MGApKZTTWrfcp3bJ-StZCG7%3DetoUTLtB0RfqePWvrwQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Nov 13, 2020 at 10:41 AM K. Moon <km...@chromium.org> wrote:

With regards to (2), overall opinion seems to be leaning towards making this almost a "no comment" issue (whichever style-compliant choice the author makes should be respected), but I believe there have been at least one or two comments in this thread about preferring to omit in certain cases, which I think (unfairly?) privileges the historical option over the non-historical option.

While I personally prefer to omit, I wouldn't cite this in a review.  For reviewers who do, the guidance would be the same as other cases:

• Reviewers are allowed to request things that go beyond official guidance.
• For any suggestion, reviewers should cite relevant official guidance or make it clear there is none.
• For any suggestion without guidance, reviewers should make the rationale for their opinion clear.
• Authors are not bound to follow reviewer comments that go beyond official guidance.
• Intractable disagreements call for escalation, e.g. to a neutral-but-authoritative third party.

So a sample review comment calling for brace omission might be: "Nit: This is OK per the style guide.  In this file, no other single-line conditionals have braces.  For consistency, please either omit braces here or fix the other cases as well (possibly as a separate CL)."

PK

[Daniel Cheng]

On Fri, Nov 13, 2020 at 10:49 AM Nico Weber <tha...@chromium.org> wrote:

On Fri, Nov 13, 2020 at 1:41 PM K. Moon <km...@chromium.org> wrote:

I think this decision is a bit different than other possible deviations from the style guide, in that the style guide explicitly states that projects can make a choice. "Some projects require curly braces always," is an extremely unusual statement to see in the style guide, which generally doesn't suggest project-specific carve-outs. It also suggests opting out of the historical rule, but not vice versa.

That said, unless there's a sudden outpouring of support to the contrary, I don't think there's consensus for picking a single style on this, so let's consider that question decided.

I think there's still two more decisions to make in this thread:

1. What should tooling do?

If someone has bandwidth to try and implement something in clang-format, then they should implement something in clang-format :) IIRC adding this was harder than it at first seemed though. 

If someone *is* interested in trying to make this work in clang-format, I can send the background info I got when I previously asked about how to implement this. But see below. 

1. How should reviewers handle disagreements about whether or not to omit braces?

With regards to (1), it sounds like we'd like to configure clang-tidy to (mostly) enforce the style guide rule, which SGTM.

It's imho not a great use of time for reviewers to argue about this. The goal of code review is to make code better, and reviewers should try to ask themselves if they're adding value worth the back-and-forth overhead.

Personally I'd even say it's not worth it to make clang-tidy complain about it; if we add too many chatty diags to clang-tidy people will learn to ignore tricium comments. But I'm willing to have my mind changed on this point. https://chromium.googlesource.com/chromium/src/+/master/docs/clang_tidy.md#Adding-a-new-check describes the process for adding checks to clang-tidy, if you want to give it a try.

+1

 

With regards to (2), overall opinion seems to be leaning towards making this almost a "no comment" issue (whichever style-compliant choice the author makes should be respected), but I believe there have been at least one or two comments in this thread about preferring to omit in certain cases, which I think (unfairly?) privileges the historical option over the non-historical option.

On Fri, Nov 13, 2020 at 7:06 AM Nico Weber <tha...@chromium.org> wrote:

Like Peter said, any argument here that's not chromium-specific in some way violates the "we're trying to follow the google style guide" default.

On Fri, Nov 13, 2020 at 10:02 AM Igor Bukanov <ig...@vivaldi.com> wrote:

On Thursday, November 12, 2020 at 6:10:13 PM UTC+1 Vladimir Levin wrote:

> I think verbosity would be my main argument here. I don't particularly mind braces, but saying

>

> if (!client)

>  return;

>

> is clear enough for me

This argument suggest to treat "if (condition) control_transfer;" differently from the "if" followed by non-jumpy code. Perhaps the rule should be that for the control transfer case there should be no braces unless "if (condition)" or the transfer like "return long_expression" does not fit one line. Or at least to always require braces only for non-jumpy code but allow not to use braces for jumps.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/d7211e63-b1b6-4f0f-86cb-8c1c28f05425n%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5MGApKZTTWrfcp3bJ-StZCG7%3DetoUTLtB0RfqePWvrwQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiGBZ30V-HEe9Z_AnpcRYJxxvW%3D2hEZYgbBcOEVtM1kL1Q%40mail.gmail.com.

[K. Moon]

Peter's answer to (2) sounds good to me. As far as tooling goes, I believe this being hard in clang-format is a decent reason not to enforce it at all, but the rules are lax enough that it's possible clang-tidy wouldn't flag that many instances. +1 to experimenting first before making a final decision. (I may try to experiment with the clang-tidy check over my break, if nobody else gets to it first.)

[Greg Thompson]

I think consistency trumps personal preference ("Be consistent with existing code" is at the top of the style guide). If I'm reviewing a CL in a file that predominantly omits braces, then I ask them to be omitted in the new code. I wouldn't push back if a reviewer asked me to go one way or the other for consistency's sake.

FWIW, this has been my rule of thumb since I was convinced to stop typing {} so much (my last pre-Chromium project required them in all cases):

  if (this is one line)

   this is also one line;

  if (this is

      more than one line) {

    this is one line;

  }

  if (this is one line) {

    this is;

    more than one line;

  }

The new thing for me, then, appears to be that I need to add braces if there's an "else" or "else if" in all cases. I'll endeavor to do that in new code.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-7m-XNTWtdSzmbX4phYynzCScU4az3xbvV-KNBVS5uzew%40mail.gmail.com.