Coding Style

[Thiago Farina]

I would like to know what is the con-sense about the following coding
styles:

Consider this if clause, notice that it's without the brackets {}
around it, although some reviewers ask for putting {} around the if,
others remove it.

if (foo &&
bar &&
whatever)
DoSomething();

So should it be with or without the {} around it?

Now consider the following if clause:

if (foo->Bar(
type)) {
DoSomething();
}

Notice that |type| is indented 4 spaces and the if clause has {}
around it. Now I ask, the indentation is wrong? Shouldn't it have
brackets around it?

Finally, what is the preferred why to retrieve an element from
std::vector, either vector::at or vector::operator[]?

"The difference between this member function and member operator
function operator[] is that vector::at signals if the requested
position is out of range by throwing an out_of_range exception."

Reference: http://www.cplusplus.com/reference/stl/vector/

[Nikita Kostylev]

On Thu, Nov 11, 2010 at 4:17 PM, Thiago Farina <tfa...@chromium.org> wrote:

Consider this if clause, notice that it's without the brackets {}
around it, although some reviewers ask for putting {} around the if,
others remove it.

 if (foo &&
    bar &&
    whatever)
  DoSomething();

So should it be with or without the {} around it?

This case is clearly marked in C++ style guide.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditional

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.

--
Nikita

[Thiago Farina]

On Nov 11, 11:25 am, Nikita Kostylev <nkosty...@chromium.org> wrote:

> On Thu, Nov 11, 2010 at 4:17 PM, Thiago Farina <tfar...@chromium.org> wrote:
> > Consider this if clause, notice that it's without the brackets {}
> > around it, although some reviewers ask for putting {} around the if,
> > others remove it.
>
> >  if (foo &&
> >     bar &&
> >     whatever)
> >   DoSomething();
>
> > So should it be with or without the {} around it?
>

> This case is clearly marked in C++ style guide.http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Condit...<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Condit...>
>
> *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.*

That doesn't fix the issue. So the question is: is it Chromium a
project that "require that an if must always have an accompanying
brace." or not?

[Nikita Kostylev]

As far as I know - not, but more experienced Chromium engineers might correct me.

There are a lot of one line conditionals in source code w/o braces.

But your example clearly needs curly braces because it has complex multiline condition.

 

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

--
Nikita

[Marc-Andre Decoste]

The coding style stats that: 

• Other formatting "rules" are not required, but are fairly common across Chrome code, and thus it's probably best to stick to them unless you have a reason not to.  In particular, if existing code follows these styles, maintain them:
• Prefer (foo == 0) to (0 == foo)
• Omit {} for one-line conditional or loop bodies
• Function declaration order should match function definition order

   The way I read it, and is compatible with comments I give and receive in code reviews is that, {} around an if condition that only has one line AND the code body only has one line too (comments included) and if there is an else body is also has only one line, then yes, the {} SHOULD be omitted...

   But {} can also be present, because the rule is not strictly required, just highly recommended... But one rule that IS required is consistency. So if you are working in a file where all the one line if/loop conditions/bodies don't  have {}, then don't put them in the new code you add in there. Whereas if the rest of the file already has them everywhere (which is actually very rare in Chromium code), then don't omit them. If the file is not consistent, then it should be fixed...

BYE

MAD@

[Finnur Thorarinsson]

The style I was taught when I started and have been following ever since was that if at least one of the following is multiline...

1) the conditional

2) the 'if' clause (or body if you will)

3) one of the the 'else' clauses

... then curly braces are required for each clause.

[Greg Thompson]

On Thu, Nov 11, 2010 at 9:13 AM, Marc-Andre Decoste <m...@chromium.org> wrote:

The coding style stats that: 

• Other formatting "rules" are not required, but are fairly common across Chrome code, and thus it's probably best to stick to them unless you have a reason not to.  In particular, if existing code follows these styles, maintain them:
• Prefer (foo == 0) to (0 == foo)

Does anyone know the rationale for this?  Sticking to the latter is a good way to avoid the old if (foo = NULL) typo.  I can't think of a reason not to use it aside from "I just don't like it that way."

[Mike Pinkerton]

> Does anyone know the rationale for this?  Sticking to the latter is a good
> way to avoid the old if (foo = NULL) typo.  I can't think of a reason not to
> use it aside from "I just don't like it that way."

1) It hampers readability. Nobody thinks that way. The reader has to
continually re-parse the intent.
2) We have compiler warnings for the (foo = NULL) case, and they are turned on.

--
Mike Pinkerton
Mac Weenie
pink...@google.com

[Mark Mentovai]

Thiago Farina wrote:

I would like to know what is the con-sense about the following coding styles:

The letter of the law:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

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.

Chromium doesn’t have an “always use curly braces” rule.

However, if any part of your conditional construct, whether it’s an conditional expression, a conditioned statement, a comment, or even whitespace, takes you beyond “one line for an expression, one line for a statement, repeat as needed for else if/else,” I will insist on braces. I consider this a major aid to readability and it’s my interpretation of the style guide’s spirit even if it’s not the result under a strict interpretation.

To put that into code, you need to be able to fit this mold:

1  if (e1)
2    s1;
3  else if (e2)
4    s2;
5  else
6    s3;

Lines 3 and 4 may be duplicated as many times as needed. If you can’t fit any part of the entire group into this chain, you need curlies.

Consider this if clause, notice that it's without the brackets {} around it, although some reviewers ask for putting {} around the if, others remove it.

if (foo &&

    bar &&

    whatever)

  DoSomething();

So should it be with or without the {} around it?

With.

Now consider the following if clause:

if (foo->Bar(

        type)) {

 DoSomething();

}

Notice that |type| is indented 4 spaces and the if clause has {} around it. Now I ask, the indentation is wrong? Shouldn't it have brackets around it?

The indentation is fine (assuming type wouldn’t fit on the preceding line). Use braces.

A side point: I see a lot of people using the four-space continuation indent to put all of the arguments on a new line, when I’d prefer they put the first argument on the first line and then line all subsequent arguments up beneath the first. This is more readable. For example, don’t write this:

  if (SomeoneWroteAFunctionWithTooLongANameAndItHurtsRealBad(

          source, destination)) {

    LOG(FATAL) << "Programmer error";

  }

when you could write this instead:

  if (SomeoneWroteAFunctionWithTooLongANameAndHurtsRealBad(source,

                                                           destination)) {

    LOG(FATAL) << "Programmer error";

  }

Finally, what is the preferred why to retrieve an element from std::vector, either vector::at or vector::operator[]?

"The difference between this member function and member operator function operator[] is that vector::at signals if the requested position is out of range by throwing an out_of_range exception."

If you know that your index is within the vector’s bounds, it doesn’t make a difference. If you haven’t checked the index: if it’s OK to crash (since we don’t handle exceptions) vector::at is fine. If it’s OK to do the wrong thing (read or write memory that doesn’t belong to the vector), operator[] is fine. I think that it’s usual to check the index, though.

[Brett Wilson]

On Thu, Nov 11, 2010 at 6:15 AM, Finnur Thorarinsson
<fin...@chromium.org> wrote:
>
> The style I was taught when I started and have been following ever since was
> that if at least one of the following is multiline...
> 1) the conditional

This isn't technically one of the requirements. Sometimes {} in this
case can help readability and you should do it. Other times its less
important and you can leave it off.

Brett

[Peter Kasting]

What Brett said.  I will generally leave off braces for multi-line conditionals with single-line bodies.  Occasionally adding braces can help and I will put them in.  I do try to correct people who believe that braces are _required_ for multi-line conditionals -- it's only the body that matters.

As for at() vs. []: You should use [], not at().  We do not compile with exception support anyway, so if you are in code that could receive out-of-range indexes, you should be using DCHECK() or CHECK() (or a conditional if the out-of-range value could be a legitimate argument) to catch the problem rather than at(), which is more explicit and clear.  Most code, including all instances of at() I've ever encountered in our code, does not need to check the index at all, and at() therefore is to some degree implying some checking that isn't actually needed.

There may also be a perf difference if this is in an inner loop, but I can't imagine it actually mattering in most cases so I don't think that's a strong argument for [].

PK

[Chris Bentzel]

Have there even been arguments for always adding braces?

I have had one case where I incorrectly tried to add a second statement to an if body without adding braces - fortunately it was caught by a unit test.

A tradeoff of extra vertical space for decreased likelihood of stupid errors seems like a good one to me. 

[Peter Kasting]

On Thu, Nov 11, 2010 at 11:19 AM, Chris Bentzel <cben...@chromium.org> wrote:

Have there even been arguments for always adding braces?

We had a few discussions long ago.

The risk of doing the wrong thing (i.e. what you cited) is pretty low for single-line conditionals + bodies, somewhat higher (depending on the specific case) for multi-line conditionals, and significantly higher for multi-line bodies.  Hence the distinctions we make in the guidelines

In general, braces are visually noisy, so the cost is not just vertical space, but also slight cognitive load.  Obviously, that load is not a big deal, but it's nonzero, which is why many people prefer to avoid the braces where they're not viewed as "helpful enough".  I for one am strongly opposed to "always add braces".

PK 

[Mark Mentovai]

Chris Bentzel wrote:
> Have there even been arguments for always adding braces?
> I have had one case where I incorrectly tried to add a second statement to
> an if body without adding braces - fortunately it was caught by a unit test.
> A tradeoff of extra vertical space for decreased likelihood of stupid errors
> seems like a good one to me.

There’s this, there’s the “I have fewer decisions to make so I can
focus on what matters” point, and there’s “it makes adding statements
or otherwise maintaining the whole construct easier in the future”
point. I personally never experience the situation you bring up when
I’m writing code, but I see it all the time when debugging by
inserting printfs or LOGs at strategic decision points and wind up
having to insert braces all over the place. That really gets my goat.

Against “always require” is that it always gives you an extra line
vertically (but just one in an entire if/else if/else if/else sequence
given that we use cuddly “else,” so no big deal, right?,) and that the
opening brace would very infrequently bump a line past 80 characters
requiring creative reindenting.

I still think it ought to be a judgment call. I probably favor braces
in most cases, but I wouldn’t imagine writing a real shorty with
braces.

if (a < b)
r = -1;
else if (b > a)
r = 1;
else
r = 0;

if (a < b) {
r = -1;
} else if (b > a) {
r = 1;
} else {
r = 0;
}

Maybe it’s just what I’m accustomed to, but the former permeates my
brain more readily. (No, don’t write the above by nesting a ternary.)

[Dirk Pranke]

I personally always use braces; I actually find the *lack* of braces
makes C/C++ conditionals harder to read, but that's probably from
force of habit. Back in the day when I wrote C code for a living at
Oracle, lack of braces in existing code frequently caused maintenance
problems for me (and others).

I particularly dislike it when people put the statement on the same
line as the conditional, braces or no:

if (foo) DoSomething();

The problems with this are that it's usually impossible to set a
breakpoint on DoSomething() (so that you only break when the condition
is true) [of course, you can do this with a watchpoint], and that it
makes it more important to get branch-level code coverage instead of
line-level (and line level is usually much easier to get).

That said, I am not arguing that we change the style guide.

-- Dirk

[Greg Thompson]

On Thu, Nov 11, 2010 at 2:29 PM, Peter Kasting <pkas...@google.com> wrote:

In general, braces are visually noisy, so the cost is not just vertical space, but also slight cognitive load.

This is purely subjective, and pretty much boils down to what you're accustomed to seeing/doing.  For me, there's a higher cognitive load for no braces <shrug>.

[Mark Mentovai]

Dirk Pranke wrote:
> I particularly dislike it when people put the statement on the same
> line as the conditional, braces or no:
>
> if (foo) DoSomething();
>
> The problems with this are that it's usually impossible to set a
> breakpoint on DoSomething() (so that you only break when the condition
> is true) [of course, you can do this with a watchpoint], and that it
> makes it more important to get branch-level code coverage instead of
> line-level (and line level is usually much easier to get).

and that it makes it difficult to tell if there’s something wrong with
foo or DoSomething() if all you’ve got is a backtrace from Breakpad.

Whether it’s right or not, debugging tools are much more capable at
the line level than they are at the expression level. The style guide
permits the above as a one-liner when it “enhances readability,” but
I’d say “never do it.”

On the other hand, if you’re in some big dispatcher-like thing
structured as a switch statement, I can see a much better argument for
putting each case on a single line if it’s a readability win.

[Peter Kasting]

On Thu, Nov 11, 2010 at 12:19 PM, Mark Mentovai <ma...@chromium.org> wrote:

The style guide

permits the above as a one-liner when it “enhances readability,”

Really?  The Google C++ style guide explicitly bans this construction (I thought).

I certainly agree that it's a bad idea.  I was sad to find it in some Chrome code a month or so ago :(

PK 

[Mark Mentovai]

Peter Kasting wrote:
> On Thu, Nov 11, 2010 at 12:19 PM, Mark Mentovai <ma...@chromium.org> wrote:
>>
>> The style guide
>> permits the above as a one-liner when it “enhances readability,”
>
> Really?  The Google C++ style guide explicitly bans this construction (I
> thought).

Sadly, it’s tolerated.

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

> Short conditional statements may be written on one line if this
> enhances readability. You may use this only when the line is brief and
> the statement does not use the else clause.
>
> if (x == kFoo) return new Foo();
> if (x == kBar) return new Bar();

[Sheridan Rawlins]

On Thu, Nov 11, 2010 at 8:15 AM, Mark Mentovai <ma...@chromium.org> wrote:

Finally, what is the preferred why to retrieve an element from std::vector, either vector::at or vector::operator[]?

"The difference between this member function and member operator function operator[] is that vector::at signals if the requested position is out of range by throwing an out_of_range exception."

If you know that your index is within the vector’s bounds, it doesn’t make a difference. If you haven’t checked the index: if it’s OK to crash (since we don’t handle exceptions) vector::at is fine. If it’s OK to do the wrong thing (read or write memory that doesn’t belong to the vector), operator[] is fine. I think that it’s usual to check the index, though.

Hmm...  thinking out loud for a moment, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions states that "We do not use C++ exceptions." Should we eschew any code which has any chance of causing an exception, especially when alternatives exist?  Out of curiosity, do we compile with -fno-exceptions?  If so, does this cause strange behavior when using vector::at on out of bounds elements?

I would think that the range check and using operator[] would be the preferred choice given the decision @ google to not use exceptions.

Should choices like this be discussed in a STL-specific style guidelines of the C++ styleguide?

-SCR

[Fred Akalin]

> Hmm...  thinking out loud for a moment,
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions
> states that "We do not use C++ exceptions." Should we eschew any code which
> has any chance of causing an exception, especially when alternatives exist?
>  Out of curiosity, do we compile with -fno-exceptions?  If so, does this
> cause strange behavior when using vector::at on out of bounds elements?
> I would think that the range check and using operator[] would be the
> preferred choice given the decision @ google to not use exceptions.
> Should choices like this be discussed in a STL-specific style guidelines of
> the C++ styleguide?

I believe we do compile with -fno-exceptions. This makes any code
that throws simply print the exception and abort the program.

[Mark Mentovai]

Sheridan Rawlins wrote:
> Hmm...  thinking out loud for a moment,
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Exceptions
> states that "We do not use C++ exceptions." Should we eschew any code which
> has any chance of causing an exception, especially when alternatives exist?

We can’t get rid of exceptions from portions of the C++ standard
library that we link against as a binary. “new char[-1]” will throw
std::bad_alloc regardless of whether you build your own code with
-fno-exceptions.

>  Out of curiosity, do we compile with -fno-exceptions?

On Mac and Linux, for Chromium, yes. This isn’t necessarily consistent
across projects or platforms, though.

> If so, does this
> cause strange behavior when using vector::at on out of bounds elements?

You get an exception, and it’s unhandled, and the program aborts. In
GNU libstdc++, this exception is thrown from the binary portion of
libstdc++. This is std::__throw_out_of_range(const char*). This isn’t
particularly strange. It’s exactly the same thing as what happens when
exceptions are enabled, an exception is raised, and there isn’t a
“try” anywhere on the stack.

> I would think that the range check and using operator[] would be the
> preferred choice given the decision @ google to not use exceptions.

“Don’t use exceptions” is an oversimplification when we’re discussing
actual exception handling internals. It would be more accurate to
characterize the position as “never write throw or try; possibly build
with -fno-exceptions; accept that C++ exceptions might occur anyway;
accept that they will result in crashes.”

> Should choices like this be discussed in a STL-specific style guidelines of
> the C++ styleguide?

I doubt it. There are other documents where this discussion might be
appropriate. This isn’t really about style, it’s about how the C++
standard library works and how to use it. (It’s also exactly the sort
of thing that I suspect the kids have in mind when they say that C++
makes it too easy to shoot yourself in the foot.)

The tradeoff, when you’re not checking your indices, is between one
sort of bug (bad access, corruption or crash) and another (certain
crash, since we can’t write “catch.”) [] is prettier and
well-recognized. std::vector<>::at isn’t forbidden. We’re not going to
outlaw std::vector<>::operator[] either.

Bottom line? Check your indices.