maybe consider full bracing in the coding style

[Sorin Jianu]

In the light of the recent Apple SSL/TLS bug, I am thinking that encouraging full bracing for conditionals and loops in the Chromium code base could avoid coding bugs such as the one below :

        if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
		goto fail;
	if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
		goto fail;
		goto fail;
	if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
		goto fail;

(code snippet from http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c)

[Scott Graham]

clang-format would work too.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Alexei Svitkine]

Can our linter be taught to detect this type of inconsistent indentation and issue a presubmit warning?

[Nico Weber]

I like to think that it's unlikely that this exact bug would happen in Chromium (strict code review policy, strict testing policy, clang-format, etc). I think the lessons to take away here aren't "let's parse C in our presubmit!" or "let's require braces on all ifs", but rather:

* When reviewing code, review every line, don't just glance over the review and think "looks roughly correct, lgtm"

* Make sure your feature has good automated tests. A good quick check you can do is introduce a small bug somewhere in your code and verify that your tests catch that.

* For subtle behavior that's hard to test automatically (hint: this is rare), work with QA to get your feature added to their manual test plan.

All this are things everyone should be doing already anyhow (and at least all the people I work with do do this :-) ), so I'm not sure there's much to do here.

(If someone wants to add an automatic check for this problem, I think the right way to attack this is to add some kind of -Windent warning to clang that checks the indent of statements after ifs, whiles, and fors. This is tricky due to tabs vs spaces (the libsecurity_nss code mixes spaces and tabs freely, so something would have to infer that tabs are 4 spaces in that codebase), performance (you'd need to compute column numbers on lots of statements), and false positive rate (as with any new warning, you'd have to try it out on a large codebase such as Chromium). If someone wants to give this a try, that'd be cool.)

Nico

[Nam Nguyen]

On Feb 22, 2014 10:41 AM, "Nico Weber" <tha...@chromium.org> wrote:
>
> I like to think that it's unlikely that this exact bug would happen in Chromium (strict code review policy, strict testing policy, clang-format, etc). I think the lessons to take away here aren't "let's parse C in our presubmit!" or "let's require braces on all ifs", but rather:
>
> * When reviewing code, review every line, don't just glance over the review and think "looks roughly correct, lgtm"
> * Make sure your feature has good automated tests. A good quick check you can do is introduce a small bug somewhere in your code and verify that your tests catch that.
> * For subtle behavior that's hard to test automatically (hint: this is rare), work with QA to get your feature added to their manual test plan.

All of the above are great points. They are not mutually exclusive with braces after if statement though. Is there any short coming with requiring braces?
Nam

[Dirk Pranke]

On Sun, Feb 23, 2014 at 3:04 PM, Nam Nguyen <namn...@chromium.org> wrote:

On Feb 22, 2014 10:41 AM, "Nico Weber" <tha...@chromium.org> wrote:
>
> I like to think that it's unlikely that this exact bug would happen in Chromium (strict code review policy, strict testing policy, clang-format, etc). I think the lessons to take away here aren't "let's parse C in our presubmit!" or "let's require braces on all ifs", but rather:
>
> * When reviewing code, review every line, don't just glance over the review and think "looks roughly correct, lgtm"
> * Make sure your feature has good automated tests. A good quick check you can do is introduce a small bug somewhere in your code and verify that your tests catch that.
> * For subtle behavior that's hard to test automatically (hint: this is rare), work with QA to get your feature added to their manual test plan.

All of the above are great points. They are not mutually exclusive with braces after if statement though. Is there any short coming with requiring braces?

The general argument against requiring braces is that it adds more visual clutter to the code, making it harder to read (hence harder to maintain, etc.).

(Note that personally I prefer a coding style where braces are required; I do think issues like the apple bug, while rare, happen often enough that they're worth trying to prevent mechanically, and that whether there really is a readability impact is debatable, but the case for this is hardly open-and-shut).

-- Dirk

 

[Nico Weber]

On Sun, Feb 23, 2014 at 3:04 PM, Nam Nguyen <namn...@chromium.org> wrote:

On Feb 22, 2014 10:41 AM, "Nico Weber" <tha...@chromium.org> wrote:
>
> I like to think that it's unlikely that this exact bug would happen in Chromium (strict code review policy, strict testing policy, clang-format, etc). I think the lessons to take away here aren't "let's parse C in our presubmit!" or "let's require braces on all ifs", but rather:
>
> * When reviewing code, review every line, don't just glance over the review and think "looks roughly correct, lgtm"
> * Make sure your feature has good automated tests. A good quick check you can do is introduce a small bug somewhere in your code and verify that your tests catch that.
> * For subtle behavior that's hard to test automatically (hint: this is rare), work with QA to get your feature added to their manual test plan.

All of the above are great points. They are not mutually exclusive with braces after if statement though. Is there any short coming with requiring braces?

As this is a style question, there will be strong opinions either way. That's why we have a style guide. "Some other code base had a serious bug where one of the things that went wrong might not have gone wrong if _we_ used a different style" isn't an argument for changing our style guide I don't think :-) (And if it was, it should be changed in the google style guide, it shouldn't be a downstream chromium style thing, as we try to stick to regular google style.)

Nam

>
> All this are things everyone should be doing already anyhow (and at least all the people I work with do do this :-) ), so I'm not sure there's much to do here.
>
> (If someone wants to add an automatic check for this problem, I think the right way to attack this is to add some kind of -Windent warning to clang that checks the indent of statements after ifs, whiles, and fors. This is tricky due to tabs vs spaces (the libsecurity_nss code mixes spaces and tabs freely, so something would have to infer that tabs are 4 spaces in that codebase), performance (you'd need to compute column numbers on lots of statements), and false positive rate (as with any new warning, you'd have to try it out on a large codebase such as Chromium). If someone wants to give this a try, that'd be cool.)

(I did give this a try, write-up / work-in-progress at http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-February/035514.html )

[Dirk Pranke]

On Sun, Feb 23, 2014 at 4:02 PM, Nico Weber <tha...@chromium.org> wrote:

On Sun, Feb 23, 2014 at 3:04 PM, Nam Nguyen <namn...@chromium.org> wrote:

On Feb 22, 2014 10:41 AM, "Nico Weber" <tha...@chromium.org> wrote:
>
> I like to think that it's unlikely that this exact bug would happen in Chromium (strict code review policy, strict testing policy, clang-format, etc). I think the lessons to take away here aren't "let's parse C in our presubmit!" or "let's require braces on all ifs", but rather:
>
> * When reviewing code, review every line, don't just glance over the review and think "looks roughly correct, lgtm"
> * Make sure your feature has good automated tests. A good quick check you can do is introduce a small bug somewhere in your code and verify that your tests catch that.
> * For subtle behavior that's hard to test automatically (hint: this is rare), work with QA to get your feature added to their manual test plan.

All of the above are great points. They are not mutually exclusive with braces after if statement though. Is there any short coming with requiring braces?

As this is a style question, there will be strong opinions either way. That's why we have a style guide. "Some other code base had a serious bug where one of the things that went wrong might not have gone wrong if _we_ used a different style" isn't an argument for changing our style guide I don't think :-) (And if it was, it should be changed in the google style guide, it shouldn't be a downstream chromium style thing, as we try to stick to regular google style.)

Actually, many style guidelines are what they are *precisely* because they are intended to help prevent bugs, and so this seems like a fine argument to me; people have been making this particular argument in C-based languages for a long time, and I've seen it in other style guides many times. 

However, I do not expect the style guide to change now, and am not particularly arguing for it now :).

-- Dirk 

[Jason Robbins]

Does anybody know exactly how that defect arose?  When I see the same line twice for no good reason, I think it might be a bad merge, which could have happened after review.

Thanks,
jason!

On Feb 22, 2014 10:41 AM, "Nico Weber" <tha...@chromium.org> wrote:

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

[Anthony Berent]

I have often seen bugs caused by not using curly braces on if statements.  What typically happens is the statement is initially written as:

if (foo)

// Foo is true so we need to set y.

  y = 1;

Later something changes, so this becomes:

if (foo)

// Foo is true so we need to set x and y

  x = 1;

  y = 1;

Note that distractions like the comment I have included make this far more likely to happen. While the writer, and the code reviewer, should spot that this is wrong it is a relatively easy bug to miss. As such I strongly favour always using braces.

We don't actually have to change the Google style guide to change this (although, I would, of course, like to). It says "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always always have an accompanying brace."

[Colin Blundell]

BTW, I believe that the phrase "single-line statement" is intended to be interpreted literally in the rule that you quoted, so your example that has a comment would actually be required to have braces (at least reviewers on my CLs have taken this interpretation).

Best,

Colin

[Anthony Berent]

I certainly meant a single line body. The example given after the line I quoted in the style guide is:

if (condition)
  DoSomething();  // 2 space indent.

See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Conditionals#Conditionals

On Mon, Feb 24, 2014 at 3:13 PM, Nam Nguyen <namn...@google.com> wrote:

What are you referring to?

if (...) ...

Or

if (...)
    ....

Isnt the second multiline?

Nam

[Colin Blundell]

My understanding is that "single-line statement" is referring to the number of lines contained in the if block:

This is OK:

if (foo)

  ...

But this would need braces according to Google style (at least from how I've been told to interpret the rule):

if (foo)

  // Explanatory comment here.

  ...

On Mon, Feb 24, 2014 at 4:13 PM, Nam Nguyen <namn...@google.com> wrote:

What are you referring to?

if (...) ...

Or

if (...)
    ....

Isnt the second multiline?

Nam

On Feb 24, 2014 3:00 AM, "Colin Blundell" <blun...@chromium.org> wrote:

[Johnny Graettinger]

Another failure mode I've been bitten by is:

if (condition);

  DoSomething();  // 2 space indent.

This is very easy to miss in a review. (+1 to requiring braces).

[David Benjamin]

(From the right address this time.)

On Feb 24, 2014 10:43 AM, "David Benjamin" <davi...@google.com> wrote:

I believe clang at least will warn on that one.

[Pavel Sergeev]

Current guides have one more disadvantage -- when somebody adds line to one-line if-body he becomes author of line with condition statement because he adds opening brace in the end of this line.

Pavel Sergeev

[Jason Robbins]

Wouldn't the unguarded goto error have been caught by a lint tool or compiler?  Didn't it make the statement after it unreachable?

Thanks,

jason!

On Mon, Feb 24, 2014 at 7:43 AM, David Benjamin <davi...@google.com> wrote:

I believe clang at least will warn on that one.

On Feb 24, 2014 10:30 AM, "Johnny Graettinger" <jgraet...@chromium.org> wrote:

[Christian Biesinger]

Well gcc removed the unreachable code warning:
http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html

Apparently LLVM has an unreachable code warning, but disabled by
default (even with -W -Wall -Wextra). But the only reference I can
find for that right now is this German blog post:
http://blog.fefe.de/?ts=adf5a1ff

-christian

[Peter Kasting]

Regarding the thread as a whole: there are files and submodules within Chrome that require braces on all conditionals; if you're modifying code in those, follow the prevailing practice.  Most code does not use braces on one-line conditionals, and I am not a huge fan of either changing it all or just instating a new rule, because the amount of code affected is so large -- probably larger than with any other rule change we've ever discussed.

The only scenario in which I would approve of something like this is if we (a) made clang-format enforce it, then (b) made clang-format mandatory, then (c) ran clang-format over the entire codebase to make it consistent.

As for my personal opinion, I prefer no braces on one-line conditionals, because I do find the "noise effect" of the braces material in terms of scanning the code.  I could survive in a world with a different rule, though.

Other replies below.

On Mon, Feb 24, 2014 at 7:29 AM, Johnny Graettinger <jgraet...@chromium.org> wrote:

Another failure mode I've been bitten by is:

if (condition);

  DoSomething();  // 2 space indent.

This is very easy to miss in a review. (+1 to requiring braces).

Braces don't fix this typo, because:

if (condition); {

  DoSomething();

}

...still compiles.

 

On Mon, Feb 24, 2014 at 10:20 AM, Colin Blundell <blun...@chromium.org> wrote:

My understanding is that "single-line statement" is referring to the number of lines contained in the if block:

This is OK:

if (foo)

  ...

But this would need braces according to Google style (at least from how I've been told to interpret the rule):

if (foo)

  // Explanatory comment here.

  ...

Putting on my readability reviewer hat: yes, your interpretation is correct.  "One line" means one physical line rather than one "logical" line, so comment lines count towards the "one line".

PK

[Scott Graham]

VS has that warning on by default (4702), but build/common.gypi disables it.

I tried enabling it on just base, and it warned on these locations:

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/stringprintf.cc&l=81

https://code.google.com/p/chromium/codesearch#chromium/src/base/location.cc&l=98

https://code.google.com/p/chromium/codesearch#chromium/src/base/tracked_objects.cc&l=800

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/string_number_conversions.cc&l=97

The first three seem like they could just as easily be reorganized to be tidier.

But, the fourth NOTREACHED() and default return are (I'm guessing) why the warning was disabled. Additionally, the set of locations warned on is different in debug and release, which might make it a bit annoying to work with.

I also did a trial run with the warning re-enabled for chromium_code: https://codereview.chromium.org/176973004 

I skimmed through the warnings for debug build. These look like potentially real bugs:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/widget.cc&l=1206 (neither braces nor indent checking would help here, assuming it matters http://crbug.com/346380)

https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/process_mitigations.cc&l=265  (I note that using braces didn't help here, and it looks like a revert/merge problem http://crbug.com/346382)

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/webcrypto/shared_crypto.cc&l=392 http://crbug.com/346383

This one made me giggle https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/tcp_socket_resource_base.cc&l=198 given the function name, but it's just not implemented.

There are many cases that are |switch|s that have both a "default: NOTREACHED(); return;" and also a "NOTREACHED(); return;" after the switch body. (Personally, I don't like that style, but some people seem to.).

There's also a bunch of cases where "for(;;)" is used but then a "return Nothing" at the end of the function is also used. Presumably that was to silence a compiler complaining about no return value, so it'd have to be macro'd properly depending on whether the compiler knows that it's unreachable or not.

Overall, I'm not sure that it would be worth changing the correct-but-uninteresting locations to enable the warning. I think I'd probably support it iff all compilers diagnosed in similar ways.

[Peter Kasting]

On Mon, Feb 24, 2014 at 11:16 AM, Scott Graham <sco...@chromium.org> wrote:

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/stringprintf.cc&l=81

https://code.google.com/p/chromium/codesearch#chromium/src/base/location.cc&l=98

https://code.google.com/p/chromium/codesearch#chromium/src/base/tracked_objects.cc&l=800

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/string_number_conversions.cc&l=97

The first three seem like they could just as easily be reorganized to be tidier.

Yep.

But, the fourth NOTREACHED() and default return are (I'm guessing) why the warning was disabled.

 

This can be fixed too.  I filed http://crbug.com/346392 against myself for all four locations above, and noted what the fix for the last one would be.

 

I skimmed through the warnings for debug build. These look like potentially real bugs:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/widget.cc&l=1206 (neither braces nor indent checking would help here, assuming it matters http://crbug.com/346380)

https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/process_mitigations.cc&l=265  (I note that using braces didn't help here, and it looks like a revert/merge problem http://crbug.com/346382)

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/webcrypto/shared_crypto.cc&l=392 http://crbug.com/346383

Assigned all these so they'll get fixed.

This one made me giggle https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/tcp_socket_resource_base.cc&l=198 given the function name, but it's just not implemented.

Filed and assigned http://crbug.com/346389 .

 

There are many cases that are |switch|s that have both a "default: NOTREACHED(); return;" and also a "NOTREACHED(); return;" after the switch body. (Personally, I don't like that style, but some people seem to.).

I agree that that's not good style.  We should fix those.

There's also a bunch of cases where "for(;;)" is used but then a "return Nothing" at the end of the function is also used. Presumably that was to silence a compiler complaining about no return value, so it'd have to be macro'd properly depending on whether the compiler knows that it's unreachable or not.

Overall, I'm not sure that it would be worth changing the correct-but-uninteresting locations to enable the warning. I think I'd probably support it iff all compilers diagnosed in similar ways.

Given your findings above, it seems like this can catch some real bugs, and hopefully wouldn't be completely infeasible to enable.  I filed http://crbug.com/346399 about doing this.  Anyone passionate about this and want to drive it to completion?

PK

[Jiang Jiang]

On Mon, Feb 24, 2014 at 8:16 PM, Scott Graham <sco...@chromium.org> wrote:
> Overall, I'm not sure that it would be worth changing the
> correct-but-uninteresting locations to enable the warning. I think I'd
> probably support it iff all compilers diagnosed in similar ways.

I suppose that means we want to turn on -Wunreachable-code for clang builds?

- Jiang

[Peter Kasting]

IMO yes, and I tried to mention this on the master bug http://crbug.com/346399.

PK 

[Stefan Zager]

On Sun, Feb 23, 2014 at 8:38 PM, Jason Robbins <jrob...@google.com> wrote:

> Does anybody know exactly how that defect arose? When I see the same line
> twice for no good reason, I think it might be a bad merge, which could have
> happened after review.

For me, this is probably the most important take-away. Any project
that moves at high velocity in a single code base is susceptible to a
bad merge/rebase/cherry-pick. Linting and good style and careful code
review are of course important, but the only real safety net for a bad
merge is thorough tests.

[Isiah Meadows]

In reference to an earlier comment regarding both spaces and tabs used on a single file, there really should be a coding style guideline going one way or the other. That alone is kinda ridiculous (all my solo free time projects end up with 2 space indentation intervals because it is just easier to deal with).

[Peter Kasting]

On Tue, Feb 25, 2014 at 6:55 PM, Isiah Meadows <impi...@gmail.com> wrote:

In reference to an earlier comment regarding both spaces and tabs used on a single file, there really should be a coding style guideline going one way or the other.

There is.  In Chromium and Blink code tabs are banned.  The reference earlier was to code in a third-party library.

PK