[PATCH] clang-format: Add column layout formatting for braced lists

[Daniel Jasper]

================
Comment at: lib/Format/FormatToken.h:379
@@ +378,3 @@
+    /// \brief The total length in number of lines.
+    unsigned TotalLength;
+
----------------
Manuel Klimek wrote:
> I'd probably call that LineCount or something else with Line in it then...
Done.

================
Comment at: lib/Format/FormatToken.cpp:39
@@ +38,3 @@
+    return 0;
+  unsigned RemainingColumns = Style.ColumnLimit - State.Stack.back().Indent;
+  const ColumnFormat *Format = getColumnFormat(RemainingColumns);
----------------
Manuel Klimek wrote:
> So 'RemainingColumns' is in characters, but 'Column' below is in "entries"?
Changed to RemainingCharacters.

================
Comment at: lib/Format/FormatToken.cpp:76
@@ +75,3 @@
+  FormatToken *ItemBegin = Token->Next;
+  SmallVector<bool, 6> MustBreakBeforeItem;
+  SmallVector<unsigned, 6> ItemLengthsInLastColumn;
----------------
Manuel Klimek wrote:
> Has the magic number '6' any meaning?
No. Any hint on how to pick these?

================
Comment at: lib/Format/FormatToken.cpp:105
@@ +104,3 @@
+    bool HasRowWithSufficientColumns = false;
+    for (unsigned i = 0, e = ItemLengths.size(), Column = 0; i != e; ++i) {
+      if (MustBreakBeforeItem[i] || Column == Columns) {
----------------
Manuel Klimek wrote:
> Pulling out Columns would make this line idiomatic and easier to read for me.
Done (if I understood correctly).

================
Comment at: lib/Format/FormatToken.cpp:87-88
@@ +86,4 @@
+                           : Commas[i];
+    ItemLengths.push_back(ItemEnd->TotalLength - ItemBegin->TotalLength +
+                          ItemBegin->CodePointCount);
+    if (ItemEnd->Next && !ItemEnd->Next->HasUnescapedNewline &&
----------------
Manuel Klimek wrote:
> This needs comments - why does subtracting two different token's length give anything useful?
Read the documentation of TotalLength .. This is used in several places.

================
Comment at: lib/Format/TokenAnnotator.cpp:275
@@ -271,1 +274,3 @@
+        Left->Role.reset(new CommaSeparatedList(Style));
+      Left->getRoleAs<CommaSeparatedList>()->CommaFound(Current);
     } else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) {
----------------
Manuel Klimek wrote:
> I'd much rather have an interface for Role that contains all the "events" that can happen that are significant to roles, and use virtual methods.
What's the benefit? We already need to know that we are dealing with a CommaSeparatedList in order to create it appropriately..

I agree that there might be more generic concepts once we have more Roles and understand them.

================
Comment at: lib/Format/FormatToken.cpp:49-67
@@ +48,21 @@
+    unsigned Spaces = State.NextToken->SpacesRequiredBefore;
+    if (Item < Commas.size() && State.NextToken->Previous == Commas[Item]) {
+      if (!State.NextToken->isTrailingComment()) {
+        Spaces += Format->ColumnSizes[Column] - ItemLengths[Item];
+        ++Column;
+      }
+      ++Item;
+      if (Column == Format->Columns) {
+        Column = 0;
+        NewLine = true;
+      }
+    }
+    if (State.NextToken->MustBreakBefore) {
+      Column = 0;
+      NewLine = true;
+    }
+    if (NewLine)
+      Penalty += State.NextToken->SplitPenalty;
+    Spaces -= State.NextToken->SpacesRequiredBefore;
+    Penalty += Indenter->addTokenToState(State, NewLine, DryRun, Spaces);
+  }
----------------
Manuel Klimek wrote:
> All this needs more comments on how this works...
Cleaned up implementation and added comments.

================
Comment at: lib/Format/FormatToken.cpp:85
@@ +84,3 @@
+    const FormatToken *ItemEnd =
+        i == Commas.size() ? Token->MatchingParen->getPreviousNonComment()
+                           : Commas[i];
----------------
Manuel Klimek wrote:
> At a first glance this seems inbalanced: wouldn't ItemEnd be one to early in for the last item?
This is actually correct. An item ends (for the purpose of column alignment at the "," or before the "}". Consider

  vector<int> a = { a,  b, c,
                    dd, e };

You do not want to take the "}" into account and align "c" relative to it.

However, I discovered a related bug, changed the implementation and added tests.


http://llvm-reviews.chandlerc.com/D1457