This is an archive of the discontinued LLVM Phabricator instance.

ClangFormat - Add option to break before inheritance separation operator in class declaration
ClosedPublic

Authored by Abpostelnicu on Mar 1 2017, 12:02 AM.

Details

Summary

This modification addresses a coding style requirement that we have at Mozilla when it comes to he way how code should be written when we are dealing with class inheritance.

  • When deriving from a single base, the inherited class should be on the same line as the the inherited class.
class MyClass : public A
{
  ...
};
  • When deriving from multiple base classes, each base class should be on it's own line, the inheritance operators should also be on the same line as the base class
class MyClass
  : public X  // When deriving from more than one class, put each on its own line.
  , public Y
{

The above rules can be found in our coding style guideline.

This patch adds a special flag: "BreakBeforeInhertianceDelimiter" that by default is false and is only set with true on our coding style.

The corresponding bug for this patch is here.

I must mention that clang-format with this patch has been applied to the entire fx repo from Mozilla and no regression were triggered. You can see the result here.

Diff Detail

Repository
rL LLVM

Event Timeline

Abpostelnicu created this revision.Mar 1 2017, 12:02 AM
Abpostelnicu edited the summary of this revision. (Show Details)Mar 1 2017, 12:08 AM

tests are coming

Also added tests.

added patch against the updated repo.

sylvestre.ledru added inline comments.Mar 1 2017, 7:03 AM
docs/ClangFormatStyleOptions.rst
430 ↗(On Diff #90176)

s/is/are/

431 ↗(On Diff #90176)

s/inheritance/ inheritances/

include/clang/Format/Format.h
307 ↗(On Diff #90176)

same typo

krasimir edited reviewers, added: djasper; removed: cfe-commits.Mar 1 2017, 7:42 AM
krasimir added subscribers: krasimir, cfe-commits.
djasper edited edge metadata.Mar 1 2017, 7:51 AM

Could you please upload a diff with the entire file as context? That makes reviewing this easier.

docs/ClangFormatStyleOptions.rst
428 ↗(On Diff #90176)

Auto-generate this with docs/tools/dump_format_style.py

include/clang/Format/Format.h
309 ↗(On Diff #90176)

s/Delimiter/Colon/

Not because it is better, but just because it's more consistent with much of the rest of clang-format.

lib/Format/TokenAnnotator.cpp
950 ↗(On Diff #90176)

Maybe "InInheritanceList"?

1012 ↗(On Diff #90176)

Why would we be in an expression here?

2398 ↗(On Diff #90176)

I don't understand this comment or what the function is supposed to do. It seems to search whether there is an inheritance comma somewhere in the rest of the line.

2490 ↗(On Diff #90176)

nit: Use periods of the end of sentences and a space after //.

unittests/Format/FormatTest.cpp
1023 ↗(On Diff #90176)

I am missing tests that show the behavior when there are multiple base classes, but they do fit on one line.

1033 ↗(On Diff #90176)

What is this supposed to test?

1039 ↗(On Diff #90176)

Sure, but the comment is forcing that, so I don't know what this test does.

Abpostelnicu marked 3 inline comments as done.Mar 2 2017, 12:12 AM
Abpostelnicu marked 9 inline comments as done.Mar 2 2017, 1:51 AM
Abpostelnicu added inline comments.
lib/Format/TokenAnnotator.cpp
2398 ↗(On Diff #90176)

I've corrected the comment, if we have single inheritance we don't want to break onto the next line before the inheritance operand, that's why i'm looking for a TT_InheritanceComma, because if there is one then we are dealing with multiple inheritance.

Abpostelnicu marked an inline comment as done.

Updated patch with the proposed modifications.

djasper added inline comments.Mar 2 2017, 8:13 AM
include/clang/Format/Format.h
309 ↗(On Diff #90300)

Hm. I am still not sure about this flag and it's name. Fundamentally, this is currently controlling two different things:

  • Whether to wrap before or after colon and comma
  • Whether or not to force wraps for multiple inheritance

That's going to get us in trouble if at some point people also want other combinations of these two things. I have to alternative suggestions:

  • Add a single flag (e.g. InheritanceListWrapping) with multiple enum values describing the various choices.
  • Don't add a flag at all, but instead break inheritance lists exactly like constructor initializers. I *think* that should solve all the know use cases for now, is easier to configure for users and we can cross the bridge of naming additional flags when we have to.
lib/Format/TokenAnnotator.cpp
2400 ↗(On Diff #90300)

I don't think you need this. If you set MustBreakBefore to true for the InheritanceCommas and set NoLineBreak to true when adding the InheritanceColon on the same line, then clang-format will already do the right thing.

Fundamentally, this seems to be identical to how we wrap constructor initializer lists in Mozilla style. So I think we should also implement this the same way (if not even reusing the same implementation).

Abpostelnicu added inline comments.Mar 3 2017, 1:05 AM
include/clang/Format/Format.h
309 ↗(On Diff #90300)

I see your worries here and indeed the current flags does two things. I'm more inclined to use the num that you suggested since in this way it's easier to separate the coding style differences that somebody uses.

lib/Format/TokenAnnotator.cpp
2400 ↗(On Diff #90300)

Now that i've seen the behaviour of NoLineBreak thanks for pointing it out to me, but still correct me if i'm wrong but shouldn't i use: `NoLineBreakInOperand`.
My guess is if i use NoLineBreak, than breaking on the current line where we would have : or , would be prohibited.

djasper added inline comments.Mar 3 2017, 2:07 AM
include/clang/Format/Format.h
309 ↗(On Diff #90300)

The trouble is that adding more style options should be done very carefully. Some of the reasons here:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

So unless we know that people would want the one style for constructor initializers and the other style for inheritance lists, I'd keep both the flags and the implementation simple.

lib/Format/TokenAnnotator.cpp
2400 ↗(On Diff #90300)

Yes, that would be prohibited and that is intended. Remember that I'd set this after placing the colon on the same line (and I should have written explicitly) as the class name. After that, there must not be any further line breaks.

But again, I think we should just have the exact same behavior and implementation as for constructor initializer lists.

Abpostelnicu added inline comments.Mar 3 2017, 4:24 AM
lib/Format/TokenAnnotator.cpp
2400 ↗(On Diff #90300)

Yes we should have the exact behaviour like the constructor initialisation list, but that one is also controlled by this flag: `BreakConstructorInitializersBeforeComma`, that on our coding style is set to true.
But still the actual behaviour of initialiser list still breaks before `'` for only one item.

Before '? Can you give an example?

Before '? Can you give an example?

MY mistake, i wanted to write `:

Do you know whether that is intentional? The style guide isn't really conclusive.

djasper added inline comments.Mar 3 2017, 4:57 AM
lib/Format/TokenAnnotator.cpp
2486 ↗(On Diff #90300)

At any rate, I think this is what makes single-item ctor init lists be split. So everything except for this spot could still share the implementation.

Do you know whether that is intentional? The style guide isn't really conclusive.

Well i'm not sure, because as you said the document is not clear but i think that when we have a single initialiser it should be on the same line as the ctor declaration. In this way it would be consistent with the style for inheritance list. The actual implementation for inheritance list breaking was tailored from the BreakConstructorInitializersBeforeComma flag.

Hm. Unfortunately, this seems to have been implemented to support Webkit style and Webkit style is explicit about wanting this (https://webkit.org/code-style-guidelines/) :(.

But maybe the solution to that is to add an extra flag like AlwaysWrapInitializerList. Really not sure how best to organize this. Any thoughts? (I personally care about neither of these styles, so maybe I am not the best to judge)

At any rate, to move forward, could you remove the hasMultipleInheritance function and instead use the alternative approach discussed?

Hm. Unfortunately, this seems to have been implemented to support Webkit style and Webkit style is explicit about wanting this (https://webkit.org/code-style-guidelines/) :(.

But maybe the solution to that is to add an extra flag like AlwaysWrapInitializerList. Really not sure how best to organize this. Any thoughts? (I personally care about neither of these styles, so maybe I am not the best to judge)

At any rate, to move forward, could you remove the hasMultipleInheritance function and instead use the alternative approach discussed?

Sure will drop hasMultipleInheritance and use the variable that you mentioned and i won't touch the rest of the patch. After that i will repost it here.

corrected some format issues.

Daniel what do you think about the last version of the patch? I don't want to have this stalled since we really need this modifications in order to be able to run clang-format on our repository.

I think the patch is fine, except for the name of the flag. It is not breaking inheritance ;).

Maybe BreakBeforeInhertianceColonAndComma, but that's pretty long still. I think maybe we can shorten this to BreakBeforeInhertianceComma, as it never makes sense to break before the comma if we keep the ":" on the old line. What do you think?

Yes that's a better name :)

djasper accepted this revision.Mar 9 2017, 8:31 AM

A few nits, otherwise looks good.

include/clang/Format/Format.h
426 ↗(On Diff #91174)

Please remove "operands" and "only". I think they can be confusing.

852 ↗(On Diff #91174)

Looks like it might fit on one line now :)

lib/Format/ContinuationIndenter.cpp
355 ↗(On Diff #91174)

Can you leave a comment here:

// Don't break within the inheritance declaration unless the ":" is on a new line.
750 ↗(On Diff #91174)

Please merge these into the one about TT_CtorInitializerComma, i.e.:

if (NextNonComment->isOneOf(TT_CtorInitializerColon, TT_InheritanceColon,
                            TT_InheritanceComma))
  return State.FirstIndent + Style.ConstructorInitializerIndentWidth;
This revision is now accepted and ready to land.Mar 9 2017, 8:31 AM
kimgr added a subscriber: kimgr.Mar 9 2017, 8:40 AM

Oops, the flag is called BreakBeforeInhertianceComma with a misspelling throughout. May want to search for inhertiance throughout and see if there are more cases.

docs/ClangFormatStyleOptions.rst
531 ↗(On Diff #91174)

Tyop: Inhertiance

Abpostelnicu marked 4 inline comments as done.Mar 10 2017, 12:43 AM
Abpostelnicu marked an inline comment as done.
djasper added inline comments.Mar 10 2017, 6:30 AM
lib/Format/TokenAnnotator.cpp
2666 ↗(On Diff #91330)

Do these now fit on one line?

Abpostelnicu added inline comments.Mar 10 2017, 6:33 AM
lib/Format/ContinuationIndenter.cpp
355 ↗(On Diff #91174)

I've modified the comment since this should be valid only if Style.BreakBeforeInheritanceComma is set to true otherwise we would be in the strange case where the input where the input is:

class A : public B,
              public C {}

And this would be translated to something like:

class A 
  : public B,
    public C {}
kimgr added a comment.Mar 10 2017, 6:51 AM

A few more 'inhertiance' left, otherwise spelling looks good to me :-)

lib/Format/TokenAnnotator.cpp
679 ↗(On Diff #91330)

This still says 'Inhertiance'

950 ↗(On Diff #91330)

'Inhertiance'

Abpostelnicu marked an inline comment as done.

Fixed two spell errors.

This revision was automatically updated to reflect the committed changes.

Added to the release notes in r297721