Page MenuHomePhabricator

[clang-format] Formatting constructor initializer lists by putting them always on different lines
Needs RevisionPublic

Authored by JVApen on Nov 8 2015, 12:04 AM.

Details

Summary

Hi all,

I've been playing around with the clang for a while now and really enjoy it. Unfortunately clang-format does not yet do what I like it to do, so I started hacking it. So here is my first successful attempt to get something working.

The issue: ConstructorInitializerAllOnOneLineOrOnePerLine only works if 'If the constructor initializers don’t fit on a line', while I prefer it to always work. In other words, I use the following formatting:

Constructor()
  : a(a)
  , b(b)

Since everyone can benefit from upstreaming, I like to share my changes and get some feedback.
Here is already some of the stuff which I was uncertain about:

  • Should I keep ConstructorInitializerAllOnOneLineOrOnePerLine or rename it (currently the second one)
  • How to name the values, currently: Compact (old: false), BestFit (old: true), OnePerLine (new)
  • Is the back-ward compatibility in ScalarEnumerationTraits a good idea? (On rename most likely not)

JVApen

Diff Detail

Repository
rL LLVM

Event Timeline

JVApen updated this revision to Diff 39647.Nov 8 2015, 12:04 AM
JVApen retitled this revision from to Formatting constructor initializer lists by putting them always on different lines.
JVApen updated this object.
JVApen added a reviewer: djasper.
JVApen set the repository for this revision to rL LLVM.
JVApen added a subscriber: cfe-commits.
JVApen added a comment.Nov 8 2015, 3:50 AM
  • Is it used by a project of significant size? Yes, we use the same style at work, where I try to get this used by about 70 developers. (Which I consider already dozens of contributors) Though it is also the same style I learned at school.
  • Does it have a publicly accessible style guide? Yes, it's both an accepted by the Google Style guide as Ubuntu Unity, which do not state that you have to use the compact form. Further more the graphisoft styleguide doesn't appear to allow the condensed form.
  • Does it have a person willing to contribute and maintain patches? Maybe, I currently don't believe I'm familiar enough with the code to maintain it. Though without putting it out here (e.g. LLVM community) the answer will definitely be no. Furthermore, beside the 2 lines in TokenAnnotator.cpp, this style does not differentiate from the current implementation.

Though since you are the/a maintainer of this code, you are more qualified to give a final answer on this question.

Regardless, even if you don't want this patch upstreamed, I like to understand the codebase better as well as the considerations which lead to certain decisions for code changes. (See also the uncertainties I listed in the original post) Which at least will make my local changes similar to what they would be in case you would allow an upstream of them.

The Google and Unity style guides have explicit examples of using the one-line version. The other style guide (at least to me) isn't clear on forbidding what clang-format currently does.

My gut feeling is that it is not worth the cost of renaming and extending the current style option. If you are trying to convince ~70 people to use clang-format, there will be several things where it doesn't do 100% what the current practice is. However, this issue (like probably several of the others) is quite an infrequent issue with very limited impact on readability.

unittests/Format/FormatTest.cpp
9753

The reason we commonly accept true/false after migrating to an enum is so that the config files remain backwards compatible. If we rename the style attribute, however, more needs to be done to keep this backwards compatible. Setting the old name to true or false should set the corresponding value of the new name unless that one is explicitly set.

I actually have more thoughts on this. If we go down this road, there are many more options that people might envision. Maybe they don't want to wrap the initializer list, if there is just one initializer. Maybe they want to align the initializers somewhere after the constructor parameters:

Constructor() : aaa(aaa),
                bbb(bbb) {}

There are many options to do this, but really the difference in readability is quite small and I think it adds unnecessary complexity to clang-format.

Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 12:14 AM

Is it possible to discuss about this feature again?

This is only a corner case if you allow only 80 characters per line.
If you allow more characters (e.g. you use doxygen for inline formatting and want some space on the right) this quickly becomes very relevant, because then each initializer list gets very unreadable.

Currently it is even not possible, to reproduce the mock up example in the original implementation
(only by setting the line limit at 40 or so which is a bit unreasonable).
https://reviews.llvm.org/D32479

// When ConstructorInitializerAllOnOneLineOrOnePerLine = true:
Constructor() :
    initializer1(),
    initializer2()
{}

I also think this feature bears further consideration.

I understand that google and other coding standard currently allows this, but I feel there's a certain degree of inconsistency in their doing so.
None of the open main coding standards would recommend or allow putting multiple variable initializations/assignments per line but this is effectively what is occurring in allowing this.
After all,

MyClass::MyClass(long _state1Long, short _state2Short, int *_state3Ptr)
  : state1Long(_state1Long), state2Short(_state2Short), state3Ptr(*_state3Ptr) {
}

is conceptually much the same as

MyClass::MyClass(long _state1Long, short _state2Short, int *_state3Ptr) {
  state1Long = _state1Long; state2Short = _state2Short; state3Ptr = *_state3Ptr;
}

which you would of course never allow.
I'm sure this conceptual inconsistency is apparent to a fair number of other C++ style guide writers where a one-per-line limitation on constructor initializers is mandated. [I'm in a similar position to JVApen and have had to patch the program to order to meet this requirement at my workplace.] Anyway, just an observation on the matter.

rfairliecuninghame added a comment.EditedAug 26 2019, 2:52 PM

The excellent and extensive Poco library project (https://pocoproject.org/) is an open-source example which only places one constructor initializer per line (StyleGuide here: http://www.appinf.com/download/CppCodingStyleGuide.pdf, random example: https://github.com/pocoproject/poco/blob/master/Foundation/src/Base64Decoder.cpp).

puya added a subscriber: puya.EditedSep 30 2019, 11:17 AM

For what it's worth the original post is also the style used at our company. I am hesitant to patch clang-format as I don't want to have to maintain it so I hope this is added as an option. Or if there was a way we could add this style without having to patch clang format.

lebedev.ri retitled this revision from Formatting constructor initializer lists by putting them always on different lines to [clang-format] Formatting constructor initializer lists by putting them always on different lines.Sep 30 2019, 11:19 AM
MyDeveloperDay added a project: Restricted Project.Oct 1 2019, 12:20 AM
MyDeveloperDay requested changes to this revision.Oct 1 2019, 1:12 AM
MyDeveloperDay added a subscriber: MyDeveloperDay.

Looking at this I'm wondering if this Isn't at least partially handled by the BreakConstructorInitializersStyle in combination with ConstructorInitializerAllOnOneLineOrOnePerLine style?

I can't be exactly sure but I think BreakConstructorInitializersStyle didn't exist before 2017 D32479: clang-format: Introduce BreakConstructorInitializers option when this original patch was submitted

BreakConstructorInitializers: BeforeComma
ConstructorInitializerAllOnOneLineOrOnePerLine: true

SomeClass::Constructor() : aaaaaa(aaaaaaa), bbbbbb(bbbbbbb), cc(cc) {}

SomeClass::Constructor()
    : aaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaa,
             aaaaaaaaaaaaaaaaaaa)
    , bbbbbb(bbbbbbb)
    , cc(cc) {}
BreakConstructorInitializers: BeforeComma
ConstructorInitializerAllOnOneLineOrOnePerLine: false

SomeClass::Constructor()
    : aaaaaa(aaaaaaa)
    , bbbbbb(bbbbbbb)
    , cc(cc) {}

SomeClass::Constructor()
    : aaaaaa(aaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaa,
             aaaaaaaaaaaaaaaaaaa)
    , bbbbbb(bbbbbbb)
    , cc(cc) {}

At least the unit tests appear to be covered by using those styles?

Nit: At a minimum, this patch would need to be rebased and be a full context diff, can anyone see a use case that can't be covered with what we have?

Moving to "request changes" (really request to abandon if not necessary any longer)

This revision now requires changes to proceed.Oct 1 2019, 1:12 AM
puya added a comment.EditedOct 1 2019, 8:36 AM

Looking at this I'm wondering if this Isn't at least partially handled by the BreakConstructorInitializersStyle in combination with ConstructorInitializerAllOnOneLineOrOnePerLine style?

I’m fairly certain that’s only true when line is long enough to be broken. In which case second one ensures one per line (as opposed to different number of initializations per line) and the first one determines the style. They would not result in the requested behavior if the line is not longer than max column. (I will double check when I am in front of a computer)

Looking at this I'm wondering if this Isn't at least partially handled by the BreakConstructorInitializersStyle in combination with ConstructorInitializerAllOnOneLineOrOnePerLine style?

[...]

At least the unit tests appear to be covered by using those styles?

I checked with https://zed0.co.uk/clang-format-configurator/ and it isn't handled.

The reason is that BreakConstructorInitializersStyle only comes into play if the constructor initializers don't fit on a line.
The unit tests work because they have

Style.ColumnLimit = 60;