Curly braces in if statements

[Lutz Justen]

Hi,

Can someone please clarify the style guide on curly braces in if statements?

"In general, curly braces are not required for single-line statements"

I find the word "statement" confusing since often the whole if block is called "if statement", which is never one line. Thus, I assume a "statement" refers to the thing after the condition, i.e.

if (condition)

  statement;

Is the following fine then?

if (a)

  aa;

else if (b)

  bb;

else if (c)

  cc;

What if the condition spans several lines?

if (a && b && c &&

    d && e)

  aa;

Is this still fine without curly braces?

Thanks,

- Lutz

[Ken Rockot]

They're all fine. This is not:

if (foo)

  ReallyLongStatement(

      SpanningMultipleLines());

Thanks,

- Lutz

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAFb54np2ASEV7D5wFpmfM%3DOmSr_QBnxPDaxK8-EF56PeZ1vVCQ%40mail.gmail.com.

[Greg Thompson]

The most important thing is consistency with the surrounding code. Personally, I add braces when either the body or the conditional spans more than one line, and I generally ask for the same when reviewing code if at least some of the surrounding code does.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BapAgG-Ys-O77mpo8w9Jeo2BLcAvbLVXdCFgQGoMXKX9bwq5w%40mail.gmail.com.

[Jan van Oort]

That's how I see it being done in most industrial contexts (talking about customers of mine like Siemens, Panasonic, Airbus...), independent of language.

Sent from my BlackBerry 10 smartphone.

From: Greg Thompson Sent: Dienstag, 23. Mai 2017 15:01 To: roc...@chromium.org; Lutz Justen Reply To: g...@chromium.org Cc: Chromium-dev Subject: Re: [chromium-dev] Curly braces in if statements

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKQnr0S5E0WprATgc6MHTN3EFiH2C1yq12GkY3TUBz18T%2BcWLg%40mail.gmail.com.

[James Cayo]

That is correct from a programming sta endpoint. In prpframmimg practice, multi line statements necessitate curly brackets whereas single line statements do not and cam be thought of as inline blocks or methods.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Mark Mentovai]

Amplifying Greg’s point, considering the examples already given:

1.

if (a && b && c &&

    d && e)

  aa;

2.

if (foo)

  ReallyLongStatement(

      SpanningMultipleLines());

(2) unambiguously requires {braces} according to our style guide. I find that the unbraced form of (1) is actually substantially more difficult to read than the unbraced form of (2) because of the ragged left margin. When reviewing code, I insist on braces for both of these cases.

(For the past few years, the meticulous engineer in me normally braces everything. I won’t usually insist on this when reviewing your code, but consider it a good practice, especially in light of “goto fail.”)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKQnr0S5E0WprATgc6MHTN3EFiH2C1yq12GkY3TUBz18T%2BcWLg%40mail.gmail.com.

[James Cayo]

In most standard programming languages, the curly braces (in cases of Java, C++) are required when there is more than one statement. Otherwise, it is optional

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHk1GDwuz79ae8MgXTzWOBPbfyGN%3DEsgd0Yh8c9Oro5BANR-Fw%40mail.gmail.com.

[Anita Woodruff]

On May 23, 2017 5:00 AM, "Lutz Justen" <lju...@chromium.org> wrote:

I find the word "statement" confusing since often the whole if block is called "if statement", which is never one line.

I think that's untrue - I count 3523 instance in chromium code where the entire if-block is on one line:

Code search for all uses of if\s\(.*\).*; -f:third_party  

e.g.

if (condition) return;

I would also appreciate clarity on what counts as single-line statements. I was under the impression it was only true one-line statements like the above that were allowed without curly braces.

[Thiago Farina]

On Wed, May 24, 2017 at 2:30 PM, 'Anita Woodruff' via Chromium-dev <chromi...@chromium.org> wrote:

On May 23, 2017 5:00 AM, "Lutz Justen" <lju...@chromium.org> wrote:

I find the word "statement" confusing since often the whole if block is called "if statement", which is never one line.

I think that's untrue - I count 3523 instance in chromium code where the entire if-block is on one line:

Code search for all uses of if\s\(.*\).*; -f:third_party  

Most of these happens on v8. They have some code styling rules that diverge from the guidelines used by Chromium.

The return statement on the same line of the if clause is one example.

--

Thiago Farina

[Daniel Cheng]

I've been wondering for some time--can we remove this Chromium-specific rule? Google style allows this: see https://google.github.io/styleguide/cppguide.html#Conditionals.

Daniel

 

--

Thiago Farina

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAFenBsZhvQCdy%3DY8AXai%3DAJo77yj3xxAxVftMkbWc55HtS7NXA%40mail.gmail.com.

[Alexei Svitkine]

(From correct email this time.)

The other guideline is to be consistent with the surrounding code and codebase. (Which doesn't include them.)

For example Google style allows both "int* x;" and "int *x;" - but we shouldn't be doing "int *x;" in Chromium because it doesn't match the style of the rest of the codebase.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKpE_KF9%2BtExCtt9NewMZfavDg_oMmdcM5mZ_pUQBsuQwg%40mail.gmail.com.

[Peter Kasting]

On Wed, May 24, 2017 at 11:11 AM, Daniel Cheng <dch...@chromium.org> wrote:

On Wed, May 24, 2017 at 10:37 AM Thiago Farina <tfa...@chromium.org> wrote:

On Wed, May 24, 2017 at 2:30 PM, 'Anita Woodruff' via Chromium-dev <chromi...@chromium.org> wrote:

On May 23, 2017 5:00 AM, "Lutz Justen" <lju...@chromium.org> wrote:

I find the word "statement" confusing since often the whole if block is called "if statement", which is never one line.

I think that's untrue - I count 3523 instance in chromium code where the entire if-block is on one line:

Code search for all uses of if\s\(.*\).*; -f:third_party  

Most of these happens on v8. They have some code styling rules that diverge from the guidelines used by Chromium.

The return statement on the same line of the if clause is one example.

I've been wondering for some time--can we remove this Chromium-specific rule? Google style allows this: see https://google.github.io/styleguide/cppguide.html#Conditionals.

What Chromium-specific rule?  I don't see anything in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md banning the above construct.

AFAICT, it's purely a consistency-with-surrounding-code matter.

(Note by contrast that we explicitly say that "int *foo" is banned, use "int* foo".)

PK

[Daniel Cheng]

Hmm... I guess I assumed based on clang-format --style=Chromium's configuration.

Daniel 

[Nico Weber]

I don't remember why I made clang-format behave this way for chromium code. Maybe Google style used to do that too and then that got flipped? (On phone, can't check atm.) I agree should make that match.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKqCHpMaYJWOfKSfVgHDBr-DupHXCL7_dH%2BJWt_txP-OZA%40mail.gmail.com.

[Mark Mentovai]

It’s exceedingly rare. When you exclude other third_party-like directories that have their own local style (v8’s a big one that’s been mentioned), generated files in src/out, and restrict it to just C/C++ source in src, I count only 210.

if\s\(.*\).*; f:^src/.*\.(cc?|mm?|h)$ -f:^src/(breakpad|native_client|out|third_party|v8)/

On that basis, I’d conclude that one-liners are nearly (but not totally) contrary to local style.

[Yuri Wiitala]

IMHO, any if-statements without braces are a bad practice:

1. It's easy to introduce unintended bugs: For example, a future change might add a second statement with the same indentation, but the developer forgot to add braces around the two statements. Or, maybe a developer accidentally duplicated a statement, and the compiler did not catch the mistake (there was a HUGE SSL exploit about a year ago that leveraged such a thing)

2. It's better for readability, if you subscribe to the "more whitespace is usually better" line of thinking. Also, since branches have a cost that is typically more than other statements (i.e., those without function calls), you can loosely visualize that cost as correlating to the extra LOC.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

For the reasons above, I think "being consistent with existing coding style" would cost us more in the long-term. FWIW, we could just run clang format on existing code to make things consistent again (i.e., add braces around everything) anyway.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not. Or, at the least, we should stop banning people from using braces for one-statement blocks, should they wish to do that.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Peter Kasting]

On Thu, May 25, 2017 at 7:01 PM, Yuri Wiitala <m...@chromium.org> wrote:

2. It's better for readability, if you subscribe to the "more whitespace is usually better" line of thinking.

 

It depends on context.  In some situations this can be a win, in others a loss.  I think for simple cases like:

  if (x > 0)

    return;

...the additional line noise of braces actively hinders quickly scanning and reading the code.

Also, since branches have a cost that is typically more than other statements (i.e., those without function calls), you can loosely visualize that cost as correlating to the extra LOC.

I don't think this really makes a lot of sense.  Are you suggesting people estimate the runtime performance of code in their heads based on how many LOC there are?  That doesn't seem like a good idea.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

Or, at the least, we should stop banning people from using braces for one-statement blocks, should they wish to do that.

Those aren't banned.

PK

[Daniel Cheng]

On Thu, May 25, 2017 at 7:02 PM Yuri Wiitala <m...@chromium.org> wrote:

IMHO, any if-statements without braces are a bad practice:

1. It's easy to introduce unintended bugs: For example, a future change might add a second statement with the same indentation, but the developer forgot to add braces around the two statements. Or, maybe a developer accidentally duplicated a statement, and the compiler did not catch the mistake (there was a HUGE SSL exploit about a year ago that leveraged such a thing)

clang-formatted code should make this pretty obvious. clang-tidy also has a misleading indentation check, though I don't know if it handles this specific case.

 

2. It's better for readability, if you subscribe to the "more whitespace is usually better" line of thinking. Also, since branches have a cost that is typically more than other statements (i.e., those without function calls), you can loosely visualize that cost as correlating to the extra LOC.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

I think the extra formatting churn is true of many formatting decisions if one continually formats during development. Personally, I just don't bother formatting until I'm going to upload the CL or my edits have made the local style too hard to read.

As for adding/removing braces, we actually have a bug open to make clang-format automatically do this.

 

For the reasons above, I think "being consistent with existing coding style" would cost us more in the long-term. FWIW, we could just run clang format on existing code to make things consistent again (i.e., add braces around everything) anyway.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not. Or, at the least, we should stop banning people from using braces for one-statement blocks, should they wish to do that.

I don't think it's actually banned, but a lot of people ask for it for local consistency.  If clang-format does this automatically, that's one less thing we have to think about =)

Daniel

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKbUxDU2gg%3DgLgnA90zL%2BAfoEWSce7KxFK3UmLCer_Y9%2BA%40mail.gmail.com.

[Tommi]

Fwiw, on the subject of single-line if+body statements, like

if (Foo()) return;

It can make placing a breakpoint on the return statement require jumping through more hoops than usual.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKpA8NGZYSHk9FS8q%2BqixHbDa6qmCLtUtgPd4vumPefgOg%40mail.gmail.com.

[Jan van Oort]

That is a very good point not yet made in this discussion. 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANDN_yX-ZPaiAmk%2BG%2BnKddWCRNKdYjXkC_cpVhVoV4hjdPC8bg%40mail.gmail.com.

[Yuri Wiitala]

CIL...

2017/05/25 午後8:03 "Peter Kasting" <pkas...@chromium.org>:

On Thu, May 25, 2017 at 7:01 PM, Yuri Wiitala <m...@chromium.org> wrote:

2. It's better for readability, if you subscribe to the "more whitespace is usually better" line of thinking.

 

It depends on context.  In some situations this can be a win, in others a loss.  I think for simple cases like:

  if (x > 0)

    return;

...the additional line noise of braces actively hinders quickly scanning and reading the code.

Agreed, that context matters. For super simple if-statements like this, I would not be opposed to:

  if (x > 0) return;

Actually, I'd argue one-liners like this would help the quick-scan/reading of code; rather than the two-liner version w/o braces. Patterns like this pop-up all the time:

  for (const auto& e : list) {

    if (IsFilteredOut(e)) continue;

    Process(e);

  }

Compare to:

  for (const auto& e : list) {

    if (IsFilteredOut(e))

      continue;

    Process(e);

  }

and:

  for (const auto& e : list) {

    if (IsFilteredOut(e)) {

      continue;

    }

    Process(e);

  }

Heh. Maybe we should take a survey? ;-)

Also, since branches have a cost that is typically more than other statements (i.e., those without function calls), you can loosely visualize that cost as correlating to the extra LOC.

I don't think this really makes a lot of sense.  Are you suggesting people estimate the runtime performance of code in their heads based on how many LOC there are?  That doesn't seem like a good idea.

Absolutely not. I cringed as I wrote that sentence since, even though I used the word "loosely," I knew it might come across as too definitive. ;-) All I meant was that it could visually trigger one to be more aware and think about such things.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

Actually, I don't feel that I am overselling on this point: Both you and dcheng@ suggest coding and letting the formatter fix things later. However, that suggests that style is only important for code in the tree. What about the code in your local checkout? And during the development of a patch? Because style is meant to improve readability, understanding, and thought process, I generally try to maintain style while I am working through a code change. I use the formatter, at the end, as more of a "catch whatever I missed" tool.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

I'm remembering back years ago when I was new to Chromium and people brow-beat me (it felt) to not use braces on short conditionals. So, this is good to know for future reviews. :)

[Peter Kasting]

On Tue, May 30, 2017 at 2:39 PM, Yuri Wiitala <m...@chromium.org> wrote:

Agreed, that context matters. For super simple if-statements like this, I would not be opposed to:

  if (x > 0) return;

I'm not wildly opposed either.  There's no style rule against it, so while it's very uncommon today, people can try to do it if they think it's better and reviewers agree :)

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

Actually, I don't feel that I am overselling on this point: Both you and dcheng@ suggest coding and letting the formatter fix things later. However, that suggests that style is only important for code in the tree.

No, it just means that one need not be paranoid about catching every case before uploading.  I agree that it's better to maintain good style manually while writing.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

I'm remembering back years ago when I was new to Chromium and people brow-beat me (it felt) to not use braces on short conditionals. So, this is good to know for future reviews. :)

I've certainly been guilty of being too forceful about things like that as a past reviewer.  I've been trying to learn, over the last couple years, to be more accommodating.  :)

PK

[Sylvain Defresne]

On Tue, May 30, 2017 at 11:39 PM, Yuri Wiitala <m...@chromium.org> wrote:

CIL...

2017/05/25 午後8:03 "Peter Kasting" <pkas...@chromium.org>:

On Thu, May 25, 2017 at 7:01 PM, Yuri Wiitala <m...@chromium.org> wrote:

2. It's better for readability, if you subscribe to the "more whitespace is usually better" line of thinking.

 

It depends on context.  In some situations this can be a win, in others a loss.  I think for simple cases like:

  if (x > 0)

    return;

...the additional line noise of braces actively hinders quickly scanning and reading the code.

Agreed, that context matters. For super simple if-statements like this, I would not be opposed to:

  if (x > 0) return;

Actually, I'd argue one-liners like this would help the quick-scan/reading of code; rather than the two-liner version w/o braces. Patterns like this pop-up all the time:

  for (const auto& e : list) {

    if (IsFilteredOut(e)) continue;

    Process(e);

  }

FWIW, I find that putting the "continue" on the same line as the conditional makes the code less readable. I'm used to seeing "break"/"continue" on individual lines, and I don't immediately process that the item is skipped if "IsFilteredOut" returns true using that construct while I see it immediately when it is a separate line.

 

Compare to:

  for (const auto& e : list) {

    if (IsFilteredOut(e))

      continue;

    Process(e);

  }

and:

  for (const auto& e : list) {

    if (IsFilteredOut(e)) {

      continue;

    }

    Process(e);

  }

Heh. Maybe we should take a survey? ;-)

Also, since branches have a cost that is typically more than other statements (i.e., those without function calls), you can loosely visualize that cost as correlating to the extra LOC.

I don't think this really makes a lot of sense.  Are you suggesting people estimate the runtime performance of code in their heads based on how many LOC there are?  That doesn't seem like a good idea.

Absolutely not. I cringed as I wrote that sentence since, even though I used the word "loosely," I knew it might come across as too definitive. ;-) All I meant was that it could visually trigger one to be more aware and think about such things.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

Actually, I don't feel that I am overselling on this point: Both you and dcheng@ suggest coding and letting the formatter fix things later. However, that suggests that style is only important for code in the tree. What about the code in your local checkout? And during the development of a patch? Because style is meant to improve readability, understanding, and thought process, I generally try to maintain style while I am working through a code change. I use the formatter, at the end, as more of a "catch whatever I missed" tool.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

I'm remembering back years ago when I was new to Chromium and people brow-beat me (it felt) to not use braces on short conditionals. So, this is good to know for future reviews. :)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BN%2BEKYXx60snrZF1UouVusQe9gdAe88eX9dtesEyW%3DnprW0Vw%40mail.gmail.com.

[Anthony Berent]

On Tue, 30 May 2017 at 23:14 Peter Kasting <pkas...@chromium.org> wrote:

On Tue, May 30, 2017 at 2:39 PM, Yuri Wiitala <m...@chromium.org> wrote:

Agreed, that context matters. For super simple if-statements like this, I would not be opposed to:

  if (x > 0) return;

I'm not wildly opposed either.  There's no style rule against it, so while it's very uncommon today, people can try to do it if they think it's better and reviewers agree :)

One reason it is rare may be because clang formatting converts this to

  if (x > 0)

    return;

For some reason we have different rules for Java. For Java we allow

    if (x > 0) return;

and forbid

    if (x > 0)

        return;

which gets confusing when one is switching between languages. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Curly-braces.  Personally I, overall, in simple cases find the Java style more readable, and for complex cases like to include the curly braces.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

Actually, I don't feel that I am overselling on this point: Both you and dcheng@ suggest coding and letting the formatter fix things later. However, that suggests that style is only important for code in the tree.

No, it just means that one need not be paranoid about catching every case before uploading.  I agree that it's better to maintain good style manually while writing.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

I'm remembering back years ago when I was new to Chromium and people brow-beat me (it felt) to not use braces on short conditionals. So, this is good to know for future reviews. :)

I've certainly been guilty of being too forceful about things like that as a past reviewer.  I've been trying to learn, over the last couple years, to be more accommodating.  :)

PK

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFD4cSVHrCHDsppPJ9TWpd6c3q1%2BHBy2ATftoEBXapJVFw%40mail.gmail.com.

[John Williams]

On Wed, May 31, 2017 at 1:58 AM, Anthony Berent <abe...@chromium.org> wrote:

On Tue, 30 May 2017 at 23:14 Peter Kasting <pkas...@chromium.org> wrote:

On Tue, May 30, 2017 at 2:39 PM, Yuri Wiitala <m...@chromium.org> wrote:

Agreed, that context matters. For super simple if-statements like this, I would not be opposed to:

  if (x > 0) return;

I'm not wildly opposed either.  There's no style rule against it, so while it's very uncommon today, people can try to do it if they think it's better and reviewers agree :)

One reason it is rare may be because clang formatting converts this to

  if (x > 0)

    return;

For some reason we have different rules for Java. For Java we allow

    if (x > 0) return;

and forbid

    if (x > 0)

        return;

which gets confusing when one is switching between languages. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md#Curly-braces.  Personally I, overall, in simple cases find the Java style more readable, and for complex cases like to include the curly braces.

IMHO this is a case where what's more readable depends more or less entirely on what you're used to. Personally I like the one-line Java style and I'll use it code that's just for me, but I'll avoid it in code I plan on sharing because I know not everyone likes it. The version with indentation but no braces just makes me cringe (for the reasons Yuri mentioned). My hard and fast rule is to always use braces whenever the opening and closing braces would go on different lines.

3. It costs us quite a bit in extra development effort: While writing some code, you might be moving/rewriting parts repeatedly; and having to remember AND execute the addition/removal of braces just because you have 1→2 statements (or 2→1). If we always used braces, there is no longer anything to think about or have to change back-and-forth.

There may be something here, but I think you're overselling it.  And as you note, if we really wanted to mandate one style, we could just run clang-format, meaning people wouldn't have to worry about this anyway.  But see below also.

Actually, I don't feel that I am overselling on this point: Both you and dcheng@ suggest coding and letting the formatter fix things later. However, that suggests that style is only important for code in the tree.

No, it just means that one need not be paranoid about catching every case before uploading.  I agree that it's better to maintain good style manually while writing.

So, I suggest we begin requiring braces for all then/else blocks, regardless of whether there's only one statement in the blocks or not.

That wouldn't pass muster for the same reason we don't have a style rule requiring people not to use braces.  It's author/reviewer discretion, Chrome does not ban braces on short conditionals.

I'm remembering back years ago when I was new to Chromium and people brow-beat me (it felt) to not use braces on short conditionals. So, this is good to know for future reviews. :)

I've certainly been guilty of being too forceful about things like that as a past reviewer.  I've been trying to learn, over the last couple years, to be more accommodating.  :)

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFD4cSVHrCHDsppPJ9TWpd6c3q1%2BHBy2ATftoEBXapJVFw%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABdZ6yB46jg7sSp1MtsQYag%2B0EW%2B4bs%3DNKGvMM%3DsX-StXq%2BKRw%40mail.gmail.com.

[Christian Biesinger]

While my personal preference is also to put this on a new line, it
does come with another advantage: When debugging, you can set a
breakpoint on the body of the if, which you can't do when it's on a
single line.

Christian