Style guide clarity on C++-isms

[Benjamin Smedberg]

There are a few C++-isms which vary widely across the tree and I'd like
to clarify before we start any major refactorings.

1) Bracing of method bodies in a C++ class declaration

Currently, C++ method bodies inline within a class declaration are
documented to start on the next line, e.g.

class B
{
public:
void Method()
{
// Inline body brace is on the next line, column 2
}
};

Mozilla code widely puts the opening bracde of inline bodies such as
this on the end of the declaration line, and I want to make sure we're
in agreement to fix this.

2) indentation of visibility within a class declaration

In the above example, "public" is at the same indentation level as the
class. This is common in new code, but I want to call it out.

3) placement of the colon and commas for C++ member initializers

MyClass::MyClass()
: mMember1(foo)
, mMember2(foo)
{
// function body
}

Existing usage is all over the place. I personally have found this style
to produce the least number of merge conflicts when applying patches to
these kinds of methods, but again I want to make sure we're in agreement.

--BDS

[Robert O'Callahan]

I agree that those are the current best practices.

We have a lot of places where we write "void Method() { ... }" all on one
line for trivial setters and getters. I wonder if we should permit that.

We have a lot of places where the opening brace of a class declaration is
on the same line as the class name. We should stop doing that. Similar for
function declarations.

Rob
--
Jtehsauts tshaei dS,o n" Wohfy Mdaon yhoaus eanuttehrotraiitny eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o Whhei csha iids teoa
stiheer :p atroa lsyazye,d 'mYaonu,r "sGients uapr,e tfaokreg iyvoeunr,
'm aotr atnod sgaoy ,h o'mGee.t" uTph eann dt hwea lmka'n? gBoutt uIp
waanndt wyeonut thoo mken.o w

[Ehsan Akhgari]

On 1/7/2014, 3:53 PM, Robert O'Callahan wrote:
> I agree that those are the current best practices.
>
> We have a lot of places where we write "void Method() { ... }" all on one
> line for trivial setters and getters. I wonder if we should permit that.

I'd rather if we didn't. Often times changing the name/type of
something will make that go past the 80/100/whatever line length limit
and you'd have to indent it anyways. Plus, consistency FTW.

> We have a lot of places where the opening brace of a class declaration is
> on the same line as the class name. We should stop doing that. Similar for
> function declarations.

Agreed on both.

Cheers,
Ehsan

[Cameron McCormack]

Benjamin Smedberg wrote:
> 1) Bracing of method bodies in a C++ class declaration
>
> Currently, C++ method bodies inline within a class declaration are
> documented to start on the next line, e.g.
>
> class B
> {
> public:
> void Method()
> {
> // Inline body brace is on the next line, column 2
> }
> };
>
> Mozilla code widely puts the opening bracde of inline bodies such as
> this on the end of the declaration line, and I want to make sure we're
> in agreement to fix this.

I don't mind too much.

If we do make it same-line, it would be nice to write down how to handle
empty inline method/constructor bodies, too:

class A
{
A(int aMember)
: mMember(aMember) {
}
};

or

class A
{
A(int aMember)
: mMember(aMember) { }
};

or

class A
{
A(int aMember)
: mMember(aMember) {}
};

Also, I have seen (and written) short one-line method bodies like this:

class A
{
bool IsFlagSet() const
{ return mFlag; }
};

which I guess should be invalid with brace-on-first-line. I think I
actually prefer (and don't mind the churn that Ehsan mentions if we
rename things)

class A
{
bool IsFlagSet() const { return mFlag; }
};

if it's short enough, and if not, then we go to:

class A
{
bool IsFlagSet() const {
return mFlag;
}
};

> 2) indentation of visibility within a class declaration
>
> In the above example, "public" is at the same indentation level as the
> class. This is common in new code, but I want to call it out.
>
> 3) placement of the colon and commas for C++ member initializers
>
> MyClass::MyClass()
> : mMember1(foo)
> , mMember2(foo)
> {
> // function body
> }
>
> Existing usage is all over the place. I personally have found this style
> to produce the least number of merge conflicts when applying patches to
> these kinds of methods, but again I want to make sure we're in agreement.

Agree.

[Ehsan Akhgari]

Exactly. If we require braces on their own lines for function bodies
everywhere, we wouldn't need to solve this!

> Also, I have seen (and written) short one-line method bodies like this:
>
> class A
> {
> bool IsFlagSet() const
> { return mFlag; }
> };
>
> which I guess should be invalid with brace-on-first-line. I think I
> actually prefer (and don't mind the churn that Ehsan mentions if we
> rename things)
>
> class A
> {
> bool IsFlagSet() const { return mFlag; }
> };
>
> if it's short enough, and if not, then we go to:
>
> class A
> {
> bool IsFlagSet() const {
> return mFlag;
> }
> };

See my reply to this here. I think when in doubt here, consistency
should win. So, yay to brace on its own line for all function bodies.

Cheers,
Ehsan

[Cameron McCormack]

Ehsan Akhgari wrote:
> Exactly. If we require braces on their own lines for function bodies
> everywhere, we wouldn't need to solve this!

Are you sure? :) There are a bunch of instances of

class A
{
A(int aMember)
: mMember(aMamber)
{}
};

through the tree. Depends how the "braces on new line" rule is written,
of course.

[Ehsan Akhgari]

I guess the "close braces for function bodies on their own line" rule is
implied. But you're right, we should be explicit about that!

[Trevor Saunders]

I'd argue we should have some sort of exception here because otherwise
short ctors are unreasonably long and less readable because there's more
there. For example you go from

public:
Foo() mFoo(0) {}

to
public:
Foo()
: mFoo(0)
{
}

which takes a lot more lines for no good purpose.

Trev


> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Milan Sreckovic]

if the goal of the styles is the readability, do we know that people actually care which one of the described approaches we use, or is it also the "look, not all of these things are the same" that offends us?

For example, I find the consistency of the positioning of {} for the loops and conditionals extremely important for readability. On the other hand, the positioning of the opening { in the function definition is just about the consistency, but doesn't bother me if there are different usages.

As another example, I wish we didn't allow this kind of inlining in the first place; I find having that makes it difficult to read (it takes longer to scan the class to see what it has in it, because there are the implementation details in there.) So, it's already hard for me to read, and the white space doesn't really make any difference at that point.
--
- Milan

[Martin Thomson]

On 2014-01-09, at 08:48, Milan Sreckovic <msrec...@mozilla.com> wrote:

> For example, I find the consistency of the positioning of {} for the loops and conditionals extremely important for readability.

Outside of any real extremes, I’ve found that consistency is far more important than any personal preferences. It’s relatively easy to become accustomed to a style change, but having to switch constantly imposes a real tax. That goes for braces, whitespace, naming, language, you name it.

[Ehsan Akhgari]

On 1/9/2014, 11:48 AM, Milan Sreckovic wrote:
> if the goal of the styles is the readability, do we know that people actually care which one of the described approaches we use, or is it also the "look, not all of these things are the same" that offends us?
>
> For example, I find the consistency of the positioning of {} for the loops and conditionals extremely important for readability. On the other hand, the positioning of the opening { in the function definition is just about the consistency, but doesn't bother me if there are different usages.

It's all about consistency. All I want is for us to have one way of
writing functions as opposed to a whole bunch of exceptions for things
such as empty functions, ctors, etc.

> As another example, I wish we didn't allow this kind of inlining in the first place; I find having that makes it difficult to read (it takes longer to scan the class to see what it has in it, because there are the implementation details in there.) So, it's already hard for me to read, and the white space doesn't really make any difference at that point.

Unfortunately, C++ makes that kind of hard to avoid for performance reasons.

Cheers,
Ehsan

[Milan Sreckovic]

I didn't mean no inlining :), I was just talking about the format:

class A
{
public:
inline int hello {
return 4;
}
};

vs.

class A
{
public:
inline int hello();
};

inline int A::hello()
{
return 4;
}

--
- Milan

On 2014-01-09, at 16:21 , Ehsan Akhgari <ehsan....@gmail.com> wrote:

> ...

[Benjamin Smedberg]

On 1/13/2014 2:33 PM, Milan Sreckovic wrote:
> I didn't mean no inlining :), I was just talking about the format:
>
> class A
> {
> public:
> inline int hello {
> return 4;
> }
> };
>
> vs.
>
> class A
> {
> public:
> inline int hello();
> };
>
> inline int A::hello()
> {
> return 4;
> }

We're pretty far from the original discussion. We aren't going to moving
method bodies as part of this initial reformatting effort, so for C++
method bodies which are inline in the class, let's go with Ehsan's
strict style rules:

Method
{ // Must be on the next line
} // Must be on the following line

We can have a separate discussion about whether moving methods below the
class declaration is a good idea.

--BDS

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 12:53 PM, Robert O'Callahan <rob...@ocallahan.org> wrote:
>
> We have a lot of places where we write "void Method() { ... }" all on one
> line for trivial setters and getters. I wonder if we should permit that.

The conclusion for this question was "no", but I will ask for it to be
reconsidered.

In bug 1014377 I'm converting MFBT to Gecko style. Here are some real
one-line functions that I would have to convert to four lines each:

> static T inc(T& aPtr) { return IntrinsicAddSub<T>::add(aPtr, 1); }
> static T dec(T& aPtr) { return IntrinsicAddSub<T>::sub(aPtr, 1); }

> static T or_( T& aPtr, T aVal) { return __sync_fetch_and_or(&aPtr, aVal); }
> static T xor_(T& aPtr, T aVal) { return __sync_fetch_and_xor(&aPtr, aVal); }
> static T and_(T& aPtr, T aVal) { return __sync_fetch_and_and(&aPtr, aVal); }

> static ValueType inc(ValueType& aPtr) { return add(aPtr, 1); }
> static ValueType dec(ValueType& aPtr) { return sub(aPtr, 1); }

When it comes to questions of style, IMO clarity should trump almost
everything else, and spotting typos in functions like this is *much*
harder when they're multi-line.

Furthermore, one-liners like this are actually pretty common, and
paving the cowpaths is often a good thing to do.

Thoughts?

Nick

[Bobby Holley]

On Wed, May 28, 2014 at 9:03 PM, Nicholas Nethercote <n.neth...@gmail.com
> wrote:

> Furthermore, one-liners like this are actually pretty common, and
> paving the cowpaths is often a good thing to do.
>
> Thoughts?
>
> Nick
>

+1. In a lot of cases, it seems pretty ridiculous to use 4 lines. See
things like the OwningCompileOptions setters in jsapi.h:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#3656

[L. David Baron]

On Wednesday 2014-05-28 21:03 -0700, Nicholas Nethercote wrote:
> > static T inc(T& aPtr) { return IntrinsicAddSub<T>::add(aPtr, 1); }
> > static T dec(T& aPtr) { return IntrinsicAddSub<T>::sub(aPtr, 1); }
>
> > static T or_( T& aPtr, T aVal) { return __sync_fetch_and_or(&aPtr, aVal); }
> > static T xor_(T& aPtr, T aVal) { return __sync_fetch_and_xor(&aPtr, aVal); }
> > static T and_(T& aPtr, T aVal) { return __sync_fetch_and_and(&aPtr, aVal); }
>
> > static ValueType inc(ValueType& aPtr) { return add(aPtr, 1); }
> > static ValueType dec(ValueType& aPtr) { return sub(aPtr, 1); }
>
> When it comes to questions of style, IMO clarity should trump almost
> everything else, and spotting typos in functions like this is *much*
> harder when they're multi-line.
>

> Furthermore, one-liners like this are actually pretty common, and
> paving the cowpaths is often a good thing to do.

I'd be happy for this to be allowed; I've used this style quite a
bit, for similar reasons (clarity and density).

I also like a similar two-line style for code that doesn't fit on
one line, as in:
bool WidthDependsOnContainer() const
{ return WidthCoordDependsOnContainer(mWidth); }
bool MinWidthDependsOnContainer() const
{ return WidthCoordDependsOnContainer(mMinWidth); }
bool MaxWidthDependsOnContainer() const
{ return WidthCoordDependsOnContainer(mMaxWidth); }
from http://hg.mozilla.org/mozilla-central/file/9506880e4879/layout/style/nsStyleStruct.h#l1321
but I think that variant may be less widely used.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Ehsan Akhgari]

OK, both sound fine. Let's add them to the styles.

[Nicholas Nethercote]

I've added the "tiny methods can be written in a single line" rule.
Search for "TinyFunction" and "LargerFunction" at
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Nick