[PATCH] Major refactoring of BreakableToken.

[Alexander Kornienko]

  Nice work, now this looks even better ;)
  Please take a look at the comments inline.


================
Comment at: lib/Format/BreakableToken.h:64
@@ +63,3 @@
+
+  /// \brief Replaces the whitespace between \p LineIndex and \p LineIndex-1.
+  virtual void replaceWhitespaceBefore(unsigned LineIndex,
----------------
"between \p LineInded-1 and \p LineIndex" would read better, I think.

================
Comment at: lib/Format/BreakableToken.h:117
@@ +116,3 @@
+  BreakableBlockComment(const FormatStyle &Style, const FormatToken &Token,
+                        unsigned StartColumn, unsigned OriginaStartColumn,
+                        bool Trailing);
----------------
Maybe add a comment about what OriginalStartColumn and Trailing are? And how about Trailing -> IsTrailing?

================
Comment at: lib/Format/BreakableToken.h:132
@@ -224,4 +131,3 @@
 private:
-  unsigned OriginalStartColumn;
-  unsigned CommonPrefixLength;
-};
+  // Rearranges the whitespace between Lines[LineIndex] and Lines[LineIndex-1],
+  // so that all whitespace between the lines is accounted to Lines[i] as
----------------
Maybe "between Lines[LineIndex-1] and Lines[LineIndex]" instead?

================
Comment at: lib/Format/BreakableToken.h:133
@@ +132,3 @@
+  // Rearranges the whitespace between Lines[LineIndex] and Lines[LineIndex-1],
+  // so that all whitespace between the lines is accounted to Lines[i] as
+  // leading whitespace:
----------------
Lines[i] -> Lines[LineIndex]

================
Comment at: lib/Format/BreakableToken.h:141
@@ +140,3 @@
+  // Sets StartOfLineColumn to the intended column in which the text at
+  // Lines[LineIndex] starts.
+  void adjustWhitespace(const FormatStyle &Style, unsigned LineIndex,
----------------
I guess StartOfLineColumn is past Decoration if it's present? Please note it here.

================
Comment at: lib/Format/BreakableToken.h:145
@@ +144,3 @@
+
+  unsigned getContentStartColumn(unsigned LineIndex, unsigned TailOffset) const;
+
----------------
Could you add a comment for this method as well?

================
Comment at: lib/Format/BreakableToken.h:149
@@ +148,3 @@
+  // /* in the first line and trailing */ in the last line, and excluding all
+  // trailing whitespace between the lines.[
+  SmallVector<StringRef, 16> Lines;
----------------
You've forgotten " [".
I guess it doesn't include decorations when they are present in the original text? please note this here as well.

================
Comment at: lib/Format/BreakableToken.h:171
@@ +170,3 @@
+  // We could also support such patterns by special casing the first line
+  // instead.
+  unsigned IndentAtLineBreak;
----------------
People use '*' for item lists, and there could be a corner case when the whole comment is a bullet point list (imagine that underscores are spaces here):
/*
____* item1:
______* subitem1.1;
______* subitem1.2;
____* item2;
*/
We could probably want to retain formatting here.

================
Comment at: lib/Format/BreakableToken.cpp:90
@@ -35,1 +89,3 @@
+      // Don't break at leading whitespace.
+      Text.find_last_not_of(' ', SpaceOffset) == StringRef::npos)
     SpaceOffset = Text.find(' ', MaxSplit);
----------------
I'd leave {} for clarity.

================
Comment at: lib/Format/BreakableToken.cpp:127
@@ +126,3 @@
+
+unsigned BreakableSingleLineToken::getLineCount() const { return 1; }
+
----------------
Why not inline this?

================
Comment at: lib/Format/BreakableToken.cpp:260
@@ +259,3 @@
+    StartOfLine = Lines[LineIndex].size();
+  // FIXME: Tabs are not always 8 characters. Make configurable in the style.
+  unsigned TabExtraColumns =
----------------
And even worse: tab can be anything from 1 to tab width, depending on where it starts.

================
Comment at: lib/Format/BreakableToken.cpp:232
@@ +231,3 @@
+      // (optionally) the first whitespace.
+      int Offset = Lines[i].size() > 1 && Lines[i][1] == ' ' ? 2 : 1;
+      StartOfLineColumn[i] += Offset;
----------------
Lines[i].StartsWith(' ') ? 2 : 1

================
Comment at: lib/Format/Format.cpp:795
@@ +794,3 @@
+        Current.FormatTok.getStartOfNonWhitespace()) -
+                                   1;
+
----------------
Is it really a clang-format bug?

================
Comment at: unittests/Format/FormatTest.cpp:830
@@ +829,3 @@
+                   "   * another comment we have to break\n"
+                   "* a left commetn\n"
+                   "   */",
----------------
Typo: "commetn"

================
Comment at: unittests/Format/FormatTest.cpp:4360
@@ +4359,3 @@
+  // FIXME: Change this test to a differnt tab size than
+  // 8 once configurable.
+  EXPECT_EQ("/*\n"
----------------
Actually, this test is wrong, as it assumes that tab is 8 characters, but the usual way to interpret tabs is as a padding to the next tab stop. So in this case <space><tab> should have width of 8 characters, not 9.

================
Comment at: lib/Format/BreakableToken.cpp:201
@@ +200,3 @@
+  if (Lines.size() == 1 && Trailing) {
+    // Trailing comments can start on arbitrary column, and available
+    // horizontal space can be too small to align consecutive lines with
----------------
This comment seems to contradict the test case on line 989 now.


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

BRANCH
  breakable-tokens

ARCANIST PROJECT
  clang