[PATCH] Major refactoring of BreakableToken.

[Alexander Kornienko]

  Looks good after fixing a few nits (see the inline comments).


================
Comment at: lib/Format/BreakableToken.h:104
@@ +103,3 @@
+  //
+  // \p StartColumn specifies the column in which the comment will start
+  // after formatting.
----------------
"the comment"?

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

================
Comment at: lib/Format/BreakableToken.h:171
@@ +170,3 @@
+  // We could also support such patterns by special casing the first line
+  // instead.
+  unsigned IndentAtLineBreak;
----------------
Manuel Klimek wrote:
> 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).
Just wanted us not to miss this use-case unintentionally ;)

================
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);
----------------
Manuel Klimek wrote:
> 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 ;)
I wasn't able to find this here http://llvm.org/docs/CodingStandards.html. The only related thing I've found is:
"This document intentionally does not prescribe fixed standards for religious issues such as brace placement and space usage."
and 
"If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow."

So it's probably up to us to decide on this matter. What do you think?

================
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;
----------------
Manuel Klimek wrote:
> 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.
I was probably thinking about .startswith("* ") while writing this comment.

================
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 =
----------------
Manuel Klimek wrote:
> 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)
Isn't it what you've already implemented?


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

BRANCH
  breakable-tokens

ARCANIST PROJECT
  clang