Mir

Merge lp://qastaging/~afrantzis/mir/initializer-list-style into lp://qastaging/mir

initializer-list-style
Merge into development-branch

Proposed by Alexandros Frantzis on 2015-05-07

Status:	Rejected
Rejected by:	Alan Griffiths on 2015-05-12
Proposed branch:	lp://qastaging/~afrantzis/mir/initializer-list-style
Merge into:	lp://qastaging/mir
Diff against target:	18 lines (+4/-4) 1 file modified guides/cppguide.xml (+4/-4)
To merge this branch:	bzr merge lp://qastaging/~afrantzis/mir/initializer-list-style
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2015-05-12
Daniel van Vugt			Approve on 2015-05-12
Alberto Aguirre (community)			Approve on 2015-05-07
Alan Griffiths		2015-05-07	Needs Information on 2015-05-07
Review via email: mp+258483@code.qastaging.launchpad.net

Commit message

guides: Change multi-line initializer list style to the one we predominantly use in the code base

Description of the change

guides: Change multi-line initializer list style to the one we predominantly use in the code base

The currently approved style is:

C(int x, int y, int z) :
    x{x},
    y{y},
    z{z}
{
}

$ grep -R ") : *$" src/ include/ examples/ tests/ | wc -l
211

What we predominantly use is:

C(int x, int y, int z)
    : x{x},
      y{y},
      z{z}
{
}

$ grep -R "^ *: " src/ include/ examples/ tests/ | wc -l
412

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-05-07:

My personal preference is for the style we are predominantly using, because it visually separates the constructor parameter list from the initializer list, which can help in some cases:

===== Approved style =====

class C(
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer) :
    a_pointer{a_pointer},
    b_pointer{b_pointer},
    c_pointer{c_pointer}
{
}

===== Predominant style =====

class C(
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer)
    : a_pointer{a_pointer},
      b_pointer{b_pointer},
      c_pointer{c_pointer}
{
}

In case the comment formatting is messed up, take a look at http://paste.ubuntu.com/10990296/

The downside of the predominant style is that it introduces a special formatting rule (normally we would break the line after a special symbol, not before it).

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-07:

What you propose isn't quite as popular as you think:

$ grep -R "^*: " src/ include/ examples/ tests/ | wc -l
385

Not that this changes the conclusion.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-07:

> My personal preference is for the style we are predominantly using, because it
> visually separates the constructor parameter list from the initializer list,

My personal preference is for what we have in the guide (at least in this case - I disagree with a number of others). Lines that start with a type already look sufficiently different to those that start with a variable name.

...
> ===== Predominant style =====
>
> class C(
> std::shared_ptr<int> const& a_pointer,
> std::shared_ptr<int> const& a_pointer,
> std::shared_ptr<int> const& a_pointer)
> : a_pointer{a_pointer},
> b_pointer{b_pointer},
> c_pointer{c_pointer}

> The downside of the predominant style is that it introduces a special
> formatting rule (normally we would break the line after a special symbol, not
> before it).

The other anomaly it introduces is lines like "b_pointer{b_pointer}," indented to 4n+2 instead of the 4n that we have everywhere else.

Anyone for:

class C(
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer)
  : a_pointer{a_pointer},
    b_pointer{b_pointer},
    c_pointer{c_pointer}
{
}

;)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-05-07:

I can live with either way, and have started to like both of these colon-after-parenthesis styles as acceptable:
A(
    int a,
    float b,
    void* c) :
    mem1(a),
    mem2(b),
    mem3(c)
{
}

or, alternatively:
A(int a, float b, void* c) :
    mem1(a),
    mem2(b),
    mem3(c)
{
}

This is mostly because everything is aligned to the 4-space indents.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-07:

> This is mostly because everything is aligned to the 4-space indents.

So is this:

A(
    int a,
    float b,
    void* c)
    :
    mem1(a),
    mem2(b),
    mem3(c)
{

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3752/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-07:

*Needs Discussion*

I have a preference for what the guide says now (and have several editors set up to write that way).

But what is proposed is more common in the Mir codebase (and I guess others are set up for that).

I don't think anyone has trouble with the code we have which includes both options (and also an "aligned on any column" approach).

Maybe we should just "legalize" existing practice and allow all these options?

review: Needs Information

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-05-07:

I am ok with allowing both of the options in the original description.

I seem to have small a preference for:

===== Predominant style =====

class C(
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer,
    std::shared_ptr<int> const& a_pointer)
    : a_pointer{a_pointer},
      b_pointer{b_pointer},
      c_pointer{c_pointer}
{
}

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-05-07:

No preference one way or another.

What I do prefer is for formatting to be automatic (maybe clang-format) so that I don't even have to think about this at all.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-05-07:

Actually clang-format only supports that is proposed here. So I guess I prefer that :)

Also it looks like all other major style guides like Google, WebKit, Mozilla, LLVM use the style proposed here.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-08:

> Actually clang-format only supports that is proposed here. So I guess I prefer
> that :)

That's predicated on clang-format being /the only/ tool under consideration. I don't want to disparage what is a clearly nice piece of work, but it is only /the latest/ tool under consideration.

If in a few weeks we looked at another-format and it supported different formatting options then would your opinion change?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-05-08:

"If in a few weeks we looked at another-format and it supported different formatting options then would your opinion change?"

Possibly - leaning towards allowing both styles. Mostly I just want a path that allows us to move into automatic formatting with whichever tool we may use.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-05-11:

> > Actually clang-format only supports that is proposed here. So I guess I
> prefer
> > that :)
>
> That's predicated on clang-format being /the only/ tool under consideration. I
> don't want to disparage what is a clearly nice piece of work, but it is only
> /the latest/ tool under consideration.
>
> If in a few weeks we looked at another-format and it supported different
> formatting options then would your opinion change?

Yes. I don't think that either option is unequivocally, significantly better, so if we were considering a tool that only supported the currently-mandated layout I would be fine with that.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-05-11:

I like the :-on-separate-line style; I don't think it has any significant disadvantages - it doesn't materially increase the indent, and its indent is stable under renames/refactor. In the case of multi-line constructor declarations it makes it easier to distinguish where the constructor arguments end and active code begins.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-05-11:

I would also be happy with this format (which scales better in diffs of one-line changes):

C(int x, int y, int z)
    : x{x}
    , y{y}
    , z{z}
{
}

But generally we all accept any of these styles, have done in the past and will continue to in future. Strictly enforcing a single style in the guide is not realistic or helpful. Frankly my favourite parts of the style guide are guidelines Google wrote in there originally and mir-team has deleted, so we're never going to totally agree.

P.S. 'x{x}' doesn't work in some cases such as explicit constructors so any examples should also admit that sometimes you need to use 'w(a,b,c)' where 'w{a,b,c}' is invalid.

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-11: