[PATCH] Major refactoring of BreakableToken.

[Manuel Klimek]

================
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,
----------------
Alexander Kornienko wrote:
> "between \p LineInded-1 and \p LineIndex" would read better, I think.
Done.

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

Noticed that  'Trailing' is also misleading. Renamed and inverted logic.

================
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
----------------
Alexander Kornienko wrote:
> Maybe "between Lines[LineIndex-1] and Lines[LineIndex]" instead?
Done.

================
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:
----------------
Alexander Kornienko wrote:
> Lines[i] -> Lines[LineIndex]
Done.

================
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,
----------------
Alexander Kornienko wrote:
> I guess StartOfLineColumn is past Decoration if it's present? Please note it here.
Does that clarify?

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

================
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;
----------------
Alexander Kornienko wrote:
> You've forgotten " [".
> I guess it doesn't include decorations when they are present in the original text? please note this here as well.
Done.

================
Comment at: lib/Format/BreakableToken.h:171
@@ +170,3 @@
+  // We could also support such patterns by special casing the first line
+  // instead.
+  unsigned IndentAtLineBreak;
----------------
Alexander Kornienko wrote:
> 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.
This FIXME is not about changing the formatting, but where to break if one of the subitems protrudes the line.
Your example would actually support the idea to make the breakpoint only dependent on the line from which you're breaking (maybe with an exception for the first line to support the funny pattern).

================
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);
----------------
Alexander Kornienko wrote:
> I'd leave {} for clarity.
I think llvm style is to not put in the {}s.
(otherwise I personally would put them in *everywhere* for clarity ;)

================
Comment at: lib/Format/BreakableToken.cpp:127
@@ +126,3 @@
+
+unsigned BreakableSingleLineToken::getLineCount() const { return 1; }
+
----------------
Alexander Kornienko wrote:
> Why not inline this?
Because it doesn't matter and just clutters the interface description?

================
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
----------------
Alexander Kornienko wrote:
> This comment seems to contradict the test case on line 989 now.
Done.

================
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;
----------------
Alexander Kornienko wrote:
> Lines[i].StartsWith(' ') ? 2 : 1
Nope (note that we're looking at the *second* character). But if NeedsStar we already know that the first is a star, so I changed it to
Lines[i].startswith("* ") ? 2 : 1;
This is slightly more prone to error in the future, but YAGNI applies.

================
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 =
----------------
Alexander Kornienko wrote:
> And even worse: tab can be anything from 1 to tab width, depending on where it starts.
That is up for discussion :) (but not on this CL)

================
Comment at: lib/Format/Format.cpp:795
@@ +794,3 @@
+        Current.FormatTok.getStartOfNonWhitespace()) -
+                                   1;
+
----------------
Alexander Kornienko wrote:
> Is it really a clang-format bug?
Yes. I've manually reformatted this line in the meantime.

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

================
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"
----------------
Alexander Kornienko wrote:
> 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.
Ok, looked at the kernel code and convinced myself. (Linus' email sounded to me like he meant tabs are always 8 spaces, but apparently he didn't :P).

Found a different (related) but and added a fixme...


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