Improved formatting of constructor initializers

Added option to put each constructor initializer on its own line
if not all initializers fit on a single line. Enabling this for
Google style now as the style guide (arguable) suggests it. Not
sure whether we also want it for LLVM.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@172196 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Format/Format.h b/include/clang/Format/Format.h
index 0bafd6c7730ce8e1f6ed4aeecc46b39c8eda754c..e3a066e9d83a5688b6d6d07b1f4707eb54d868c7 100644 (file)
--- a/include/clang/Format/Format.h
+++ b/include/clang/Format/Format.h
@@ -56,6 +56,10 @@ struct FormatStyle {
   /// \brief The number of spaces to before trailing line comments.
   unsigned SpacesBeforeTrailingComments;
 
+  /// \brief If the constructor initializers don't fit on a line, put each
+  /// initializer on its own line. 
+  bool ConstructorInitializerAllOnOneLineOrOnePerLine;
+
   /// \brief Add a space in front of an Objective-C protocol list, i.e. use
   /// Foo <Protocol> instead of Foo<Protocol>.
   bool ObjCSpaceBeforeProtocolList;

diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp
index 4a2e16b112d38a5d8cbc83652185160bb8bb794a..a09f9d0e116255ac551ffcac3e01df6f6325e2f4 100644 (file)
--- a/lib/Format/Format.cpp
+++ b/lib/Format/Format.cpp
@@ -105,6 +105,7 @@ FormatStyle getLLVMStyle() {
   LLVMStyle.SplitTemplateClosingGreater = true;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
+  LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.ObjCSpaceBeforeReturnType = true;
   return LLVMStyle;
@@ -119,6 +120,7 @@ FormatStyle getGoogleStyle() {
   GoogleStyle.SplitTemplateClosingGreater = false;
   GoogleStyle.IndentCaseLabels = true;
   GoogleStyle.SpacesBeforeTrailingComments = 2;
+  GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
   GoogleStyle.ObjCSpaceBeforeProtocolList = false;
   GoogleStyle.ObjCSpaceBeforeReturnType = false;
   return GoogleStyle;
@@ -165,6 +167,26 @@ static void replacePPWhitespace(
                                        NewLineText + std::string(Spaces, ' ')));
 }
 
+/// \brief Checks whether the (remaining) \c UnwrappedLine starting with
+/// \p RootToken fits into \p Limit columns.
+bool fitsIntoLimit(const AnnotatedToken &RootToken, unsigned Limit) {
+  unsigned Columns = RootToken.FormatTok.TokenLength;
+  bool FitsOnALine = true;
+  const AnnotatedToken *Tok = &RootToken;
+  while (!Tok->Children.empty()) {
+    Tok = &Tok->Children[0];
+    Columns += (Tok->SpaceRequiredBefore ? 1 : 0) + Tok->FormatTok.TokenLength;
+    // A special case for the colon of a constructor initializer as this only
+    // needs to be put on a new line if the line needs to be split.
+    if (Columns > Limit ||
+        (Tok->MustBreakBefore && Tok->Type != TT_CtorInitializerColon)) {
+      FitsOnALine = false;
+      break;
+    }
+  };
+  return FitsOnALine;
+}
+
 class UnwrappedLineFormatter {
 public:
   UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr,
@@ -190,7 +212,7 @@ public:
     LineState State;
     State.Column = FirstIndent;
     State.NextToken = &RootToken;
-    State.Stack.push_back(ParenState(FirstIndent + 4, FirstIndent, 0, false));
+    State.Stack.push_back(ParenState(FirstIndent + 4, FirstIndent));
     State.ForLoopVariablePos = 0;
     State.LineContainsContinuedForLoopSection = false;
     State.StartOfLineLevel = 1;
@@ -206,6 +228,13 @@ public:
         unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
         unsigned Break = calcPenalty(State, true, NoBreak);
         addTokenToState(Break < NoBreak, false, State);
+        if (State.NextToken != NULL &&
+            State.NextToken->Parent->Type == TT_CtorInitializerColon) {
+          if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine &&
+              !fitsIntoLimit(*State.NextToken,
+                             getColumnLimit() - State.Column - 1))
+            State.Stack.back().BreakAfterComma = true;
+        }
       }
     }
     return State.Column;
@@ -213,10 +242,9 @@ public:
 
 private:
   struct ParenState {
-    ParenState(unsigned Indent, unsigned LastSpace, unsigned FirstLessLess,
-               bool BreakBeforeClosingBrace)
-        : Indent(Indent), LastSpace(LastSpace), FirstLessLess(FirstLessLess),
-          BreakBeforeClosingBrace(BreakBeforeClosingBrace) {}
+    ParenState(unsigned Indent, unsigned LastSpace)
+        : Indent(Indent), LastSpace(LastSpace), FirstLessLess(0),
+          BreakBeforeClosingBrace(false), BreakAfterComma(false) {}
 
     /// \brief The position to which a specific parenthesis level needs to be
     /// indented.
@@ -242,6 +270,8 @@ private:
     /// was a newline after the beginning left brace.
     bool BreakBeforeClosingBrace;
 
+    bool BreakAfterComma;
+
     bool operator<(const ParenState &Other) const {
       if (Indent != Other.Indent)
         return Indent < Other.Indent; 
@@ -249,7 +279,9 @@ private:
         return LastSpace < Other.LastSpace;
       if (FirstLessLess != Other.FirstLessLess)
         return FirstLessLess < Other.FirstLessLess;
-      return BreakBeforeClosingBrace;
+      if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace)
+        return BreakBeforeClosingBrace;
+      return BreakAfterComma;
     }
   };
 
@@ -416,7 +448,7 @@ private:
         NewIndent = 4 + State.Stack.back().LastSpace;
       }
       State.Stack.push_back(
-          ParenState(NewIndent, State.Stack.back().LastSpace, 0, false));
+          ParenState(NewIndent, State.Stack.back().LastSpace));
     }
 
     // If we encounter a closing ), ], } or >, we can remove a level from our
@@ -498,6 +530,10 @@ private:
     if (!NewLine && State.NextToken->Parent->is(tok::semi) &&
         State.LineContainsContinuedForLoopSection)
       return UINT_MAX;
+    if (!NewLine && State.NextToken->Parent->is(tok::comma) &&
+        State.NextToken->Type != TT_LineComment &&
+        State.Stack.back().BreakAfterComma)
+      return UINT_MAX;
 
     unsigned CurrentPenalty = 0;
     if (NewLine) {
@@ -1250,8 +1286,12 @@ public:
         unsigned Indent = formatFirstToken(Annotator.getRootToken(),
                                            TheLine.Level, TheLine.InPPDirective,
                                            PreviousEndOfLineColumn);
-        bool FitsOnALine = fitsOnALine(Annotator.getRootToken(), Indent,
-                                       TheLine.InPPDirective);
+        unsigned Limit = Style.ColumnLimit - (TheLine.InPPDirective ? 1 : 0) -
+                         Indent;
+        // Check whether the UnwrappedLine can be put onto a single line. If
+        // so, this is bound to be the optimal solution (by definition) and we
+        // don't need to analyze the entire solution space.
+        bool FitsOnALine = fitsIntoLimit(Annotator.getRootToken(), Limit);
         UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent,
                                          FitsOnALine, Annotator.getLineType(),
                                          Annotator.getRootToken(), Replaces,
@@ -1300,29 +1340,6 @@ private:
     UnwrappedLines.push_back(TheLine);
   }
 
-  bool fitsOnALine(const AnnotatedToken &RootToken, unsigned Indent,
-                   bool InPPDirective) {
-    // Check whether the UnwrappedLine can be put onto a single line. If so,
-    // this is bound to be the optimal solution (by definition) and we don't
-    // need to analyze the entire solution space.
-    unsigned Columns = Indent + RootToken.FormatTok.TokenLength;
-    bool FitsOnALine = true;
-    const AnnotatedToken *Tok = &RootToken;
-    while (Tok != NULL) {
-      Columns += (Tok->SpaceRequiredBefore ? 1 : 0) +
-                 Tok->FormatTok.TokenLength;
-      // A special case for the colon of a constructor initializer as this only
-      // needs to be put on a new line if the line needs to be split.
-      if (Columns > Style.ColumnLimit - (InPPDirective ? 1 : 0) ||
-          (Tok->MustBreakBefore && Tok->Type != TT_CtorInitializerColon)) {
-        FitsOnALine = false;
-        break;
-      }
-      Tok = Tok->Children.empty() ? NULL : &Tok->Children[0];
-    }
-    return FitsOnALine;
-  }
-
   /// \brief Add a new line and the required indent before the first Token
   /// of the \c UnwrappedLine if there was no structural parsing error.
   /// Returns the indent level of the \c UnwrappedLine.

diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index 7740e10b144c494367c1cd44513e3fe34049ce61..e60ed3722e2f1e704139c1ce5f891392f7ef825d 100644 (file)
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -649,11 +649,26 @@ TEST_F(FormatTest, FormatsAwesomeMethodCall) {
 
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
+  verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
+               getLLVMStyleWithColumns(45));
+  verifyFormat("Constructor()\n"
+               "    : Inttializer(FitsOnTheLine) {}",
+               getLLVMStyleWithColumns(44));
 
   verifyFormat(
       "SomeClass::Constructor()\n"
       "    : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}");
 
+  verifyFormat(
+      "SomeClass::Constructor()\n"
+      "    : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
+      "      aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}");
+  verifyGoogleFormat(
+      "SomeClass::Constructor()\n"
+      "    : aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
+      "      aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
+      "      aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}");
+
   verifyFormat(
       "SomeClass::Constructor()\n"
       "    : aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"