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

[Abpostelnicu]

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.

[sylvestre.ledru]

tests are coming

[Abpostelnicu]

Also added tests.

[Abpostelnicu]

added patch against the updated repo.

[sylvestre.ledru]

docs/ClangFormatStyleOptions.rst

430

s/is/are/

431

s/inheritance/ inheritances/

include/clang/Format/Format.h

307

same typo

[djasper]

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

docs/ClangFormatStyleOptions.rst

428

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

include/clang/Format/Format.h

309

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

Maybe "InInheritanceList"?

1012

Why would we be in an expression here?

2407

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.

2495

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

unittests/Format/FormatTest.cpp

1046

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

1056

What is this supposed to test?

1062

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

[Abpostelnicu]

lib/Format/TokenAnnotator.cpp

2407

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]

Updated patch with the proposed modifications.

[djasper]

include/clang/Format/Format.h

309

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

2410

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]

include/clang/Format/Format.h

309

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

2410

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]

include/clang/Format/Format.h

309

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

2410

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]

lib/Format/TokenAnnotator.cpp

2410

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.

[djasper]

Before '? Can you give an example?

[Abpostelnicu]

In D30487#691506, @djasper wrote:

Before '? Can you give an example?

MY mistake, i wanted to write `:

[djasper]

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

[djasper]

lib/Format/TokenAnnotator.cpp

2494

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.

[Abpostelnicu]

In D30487#691517, @djasper wrote:

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.

[djasper]

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?

[Abpostelnicu]

In D30487#691535, @djasper wrote:

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.

[Abpostelnicu]

corrected some format issues.

[Abpostelnicu]

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.

[djasper]

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?

[Abpostelnicu]

Yes that's a better name :)

[djasper]

A few nits, otherwise looks good.

include/clang/Format/Format.h

431

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

854

Looks like it might fit on one line now :)

lib/Format/ContinuationIndenter.cpp

355

Can you leave a comment here:

// Don't break within the inheritance declaration unless the ":" is on a new line.

750

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;

[kimgr]

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

Tyop: Inhertiance

[djasper]

lib/Format/TokenAnnotator.cpp

2666

Do these now fit on one line?

[Abpostelnicu]

lib/Format/ContinuationIndenter.cpp

355

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]

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

lib/Format/TokenAnnotator.cpp

679

This still says 'Inhertiance'

950

'Inhertiance'

[Abpostelnicu]

Fixed two spell errors.

[sylvestre.ledru]

Added to the release notes in r297721

docs/ClangFormatStyleOptions.rst

   A regular expression that describes comments with special meaning,
   which should not be split into lines or otherwise changed.
 
+**BreakBeforeInhertianceDelimiter** (``boolt``)
+  If ``true``, in the class inheritance expression clang-format will
+  break before operands ``:`` and ``,`` only if there is multiple
+  inheritance.
+
 **ConstructorInitializerAllOnOneLineOrOnePerLine** (``bool``)
   If the constructor initializers don't fit on a line, put each
   initializer on its own line.

include/clang/Format/Format.h

   /// which should not be split into lines or otherwise changed.
   std::string CommentPragmas;
 
+  /// \brief If ``true``, in the class inheritance expression clang-format will
+  /// break before operands ``:`` and ``,`` only if there is multiple
+  /// inheritance.
+  bool BreakBeforeInhertianceDelimiter;
+  
   /// \brief If the constructor initializers don't fit on a line, put each
   /// initializer on its own line.
   bool ConstructorInitializerAllOnOneLineOrOnePerLine;

            BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
            BreakStringLiterals == R.BreakStringLiterals &&
            ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
+           BreakBeforeInhertianceDelimiter ==
+               R.BreakBeforeInhertianceDelimiter &&
            ConstructorInitializerAllOnOneLineOrOnePerLine ==
                R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
            ConstructorInitializerIndentWidth ==

lib/Format/ContinuationIndenter.cpp

       Style.BreakConstructorInitializersBeforeComma)
     return true;
   return Previous.is(tok::comma) && !Current.isTrailingComment() &&
-         (Previous.isNot(TT_CtorInitializerComma) ||
-          !Style.BreakConstructorInitializersBeforeComma);
+         ((Previous.isNot(TT_CtorInitializerComma) ||
+          !Style.BreakConstructorInitializersBeforeComma) &&
+          (Previous.isNot(TT_InheritanceComma) ||
+          !Style.BreakBeforeInhertianceDelimiter));
 }
 
 ContinuationIndenter::ContinuationIndenter(const FormatStyle &Style,

     return State.FirstIndent + Style.ConstructorInitializerIndentWidth;
   if (NextNonComment->is(TT_CtorInitializerComma))
     return State.Stack.back().Indent;
+  if (NextNonComment->isOneOf(TT_InheritanceColon, TT_InheritanceComma))
+    return State.FirstIndent + Style.ContinuationIndentWidth;
   if (Previous.is(tok::r_paren) && !Current.isBinaryOperator() &&
       !Current.isOneOf(tok::colon, tok::comment))
     return ContinuationIndent;

       State.Stack.back().AvoidBinPacking = true;
     State.Stack.back().BreakBeforeParameter = false;
   }
+  if (Current.is(TT_InheritanceColon))
+    State.Stack.back().Indent =
+        State.FirstIndent + Style.ContinuationIndentWidth;
   if (Current.isOneOf(TT_BinaryOperator, TT_ConditionalExpr) && Newline)
     State.Stack.back().NestedBlockIndent =
         State.Column + Current.ColumnWidth + 1;

lib/Format/Format.cpp

                              false, false, false, false, false};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializersBeforeComma = false;
+  LLVMStyle.BreakBeforeInhertianceDelimiter = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";

   MozillaStyle.BinPackArguments = false;
   MozillaStyle.BreakBeforeBraces = FormatStyle::BS_Mozilla;
   MozillaStyle.BreakConstructorInitializersBeforeComma = true;
+  MozillaStyle.BreakBeforeInhertianceDelimiter = true;
   MozillaStyle.ConstructorInitializerIndentWidth = 2;
   MozillaStyle.ContinuationIndentWidth = 2;
   MozillaStyle.Cpp11BracedListStyle = false;

lib/Format/FormatToken.h

   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \

lib/Format/TokenAnnotator.cpp

     case tok::comma:
       if (Contexts.back().InCtorInitializer)
         Tok->Type = TT_CtorInitializerComma;
+      else if (Contexts.back().InInhertiance)
+        Tok->Type = TT_InheritanceComma;
       else if (Contexts.back().FirstStartOfName &&
                (Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
         Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;

     bool CanBeExpression = true;
     bool InTemplateArgument = false;
     bool InCtorInitializer = false;
+    bool InInhertiance = false;
     bool CaretFound = false;
     bool IsForEachMacro = false;
   };

                Current.Previous->is(TT_CtorInitializerColon)) {
       Contexts.back().IsExpression = true;
       Contexts.back().InCtorInitializer = true;
+    } else if (Current.Previous &&
+               Current.Previous->is(TT_InheritanceColon)) {
+      Contexts.back().IsExpression = true;
+      Contexts.back().InInhertiance = true;
     } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
       for (FormatToken *Previous = Current.Previous;
            Previous && Previous->isOneOf(tok::star, tok::amp);

          !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral);
 }
 
+// Determine if the next token from the closing scope is an inheritance token
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  // Look for a column of type TT_InheritanceComma untill we find it or
+  // we find a declaration termination
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)
+    if (nextTok->is(TT_InheritanceComma)) return true;
+  return false;
+}
+  
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
                                      const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;

       Style.BreakConstructorInitializersBeforeComma &&
       !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
     return true;
+  //Break only if we have multiple inheritance
+  if (Style.BreakBeforeInhertianceDelimiter &&
+      ((Right.is(TT_InheritanceColon) && hasMultipleInheritance(Right)) ||
+      Right.is(TT_InheritanceComma)))
+    return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
     // Raw string literals are special wrt. line breaks. The author has made a
     // deliberate choice and might have aligned the contents of the string

   if (Right.is(TT_CtorInitializerComma) &&
       Style.BreakConstructorInitializersBeforeComma)
     return true;
+  if (Left.is(TT_InheritanceComma) &&
+      Style.BreakBeforeInhertianceDelimiter)
+    return false;
+  if (Right.is(TT_InheritanceComma) &&
+      Style.BreakBeforeInhertianceDelimiter)
+    return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
       (Left.is(tok::less) && Right.is(tok::less)))
     return false;

unittests/Format/FormatTest.cpp

   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInhertianceDelimiter) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakBeforeInhertianceDelimiter = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+               "    : public X\n"
+               "    , public Y // some difficult comment\n"
+               "    , public Z {}",
+               StyleWithInheritanceBreak);
+  verifyFormat("class MyClass"
+               "  : public X\n"
+               "  , public Y // some difficult comment\n"
+               "  , public Z\n",
+               StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+               "    : public X // When deriving from more than one class, put "
+               "each on its own\n"
+               "               // line.\n"
+               "    , public Y {}",
+               StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");

   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInhertianceDelimiter)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");