[PATCH] Major refactoring of BreakableToken.

[Daniel Jasper]

  A couple of nits, but I think this looks very nice!


================
Comment at: lib/Format/BreakableToken.h:31
@@ +30,3 @@
+///
+/// FIXME: The interface seems to be so set in stone that we
+/// might want to just pull the strategy into the class, instead
----------------
s/to be //

(otherwise this sentence does not really parse for me, but might be my bad)

================
Comment at: lib/Format/BreakableToken.h:53
@@ +52,3 @@
+  /// \brief Returns a range (offset, length) at which to break the line at
+  /// \p LineIndex, if previously broken at \p TailOffset if possible not
+  /// violate \p ColumnLimit.
----------------
Sentence does not really parse (2x if)?

================
Comment at: lib/Format/BreakableToken.h:45
@@ +44,3 @@
+  /// \brief Returns the rest of the length of the line at \p LineIndex,
+  /// when broken in \p TailOffset.
+  ///
----------------
s/in/at/?

================
Comment at: lib/Format/BreakableToken.h:58
@@ +57,3 @@
+
+  /// \brief Emits the previuosly retrieved \p Split via the
+  /// \p Whitespacemanager.
----------------
s/uos/ous/

================
Comment at: lib/Format/BreakableToken.h:59
@@ -44,1 +58,3 @@
+  /// \brief Emits the previuosly retrieved \p Split via the
+  /// \p Whitespacemanager.
   virtual void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
----------------
I thinks with \p it should be \p Whitespaces.

================
Comment at: lib/Format/BreakableToken.h:114
@@ -209,3 +113,3 @@
 public:
-  BreakableBlockComment(const SourceManager &SourceMgr,
-                        const AnnotatedToken &Token, unsigned StartColumn);
+  // \brief Creates a breakable token for a line comment.
+  //
----------------
Combination of non-doxygen comments and doxygen escape-sequences seems weird.

================
Comment at: lib/Format/BreakableToken.cpp:324
@@ +323,3 @@
+  StringRef Prefix = Decoration;
+  if (LineIndex + 1 == Lines.size() && Lines[LineIndex].empty()) {
+    Prefix = "";
----------------
Remove {}.

I saw your discussion on this afterwards. I personally also don't care as much, but we should be consistent. And I think the common LLVM style is to remove this (at least I was asked to do that several times).

================
Comment at: lib/Format/UnwrappedLineParser.h:77
@@ +76,3 @@
+
+  /// \brief The raw text of the token.
+  StringRef TokenText;
----------------
I think this needs some explanation of what it actually contains, especially with respect to escaped newlines. E.g. I think it now does remove all leading escaped new lines ("test" instead of "\\\ntest"), but it still contains escaped newlines everywhere else.

================
Comment at: lib/Format/Format.cpp:833
@@ -831,3 +832,3 @@
     unsigned PositionAfterLastLineInToken = 0;
     for (unsigned LineIndex = 0; LineIndex < Token->getLineCount();
          ++LineIndex) {
----------------
I think the convention would be to use:

  for (unsigned LineIndex = 0, EndIndex = Token->getLineCount(); 
       LineIndex < EndIndex; ++LineIndex)

I know, this is not changed in this patch, but it is very close and you are actually making it hard to know that "getLineCount()" has constant runtime.

================
Comment at: lib/Format/Format.cpp:817
@@ -817,1 +816,3 @@
+          new BreakableBlockComment(Style, Current.FormatTok, StartColumn,
+                                    OriginalStartColumn, Current.Parent == 0);
       Token.reset(BBC);
----------------
We mostly don't use "== 0" checks. But I don't feel strongly.

================
Comment at: lib/Format/BreakableToken.cpp:203-204
@@ +202,4 @@
+    // and available horizontal space can be too small to align consecutive
+    // lines with the first one. We could, probably, align them to current
+    // indentation level, but now we just wrap them without stars.
+    NeedsStar = false;
----------------
The last sentence sounds like a FIXME ;-)

================
Comment at: lib/Format/BreakableToken.cpp:168
@@ +167,3 @@
+  const char *KnownPrefixes[] = { "/// ", "///", "// ", "//" };
+  for (size_t i = 0; i < llvm::array_lengthof(KnownPrefixes); ++i)
+    if (Comment.startswith(KnownPrefixes[i]))
----------------
Maybe:

 for (size_t i = 0, e = llvm::array_lengthof(KnownPrefixes); i < e; ++i)

================
Comment at: lib/Format/BreakableToken.cpp:119
@@ +118,3 @@
+  StringRef::size_type SplitPoint = getStartOfCharacter(Text, MaxSplit);
+  if (SplitPoint != StringRef::npos && SplitPoint > 1)
+    // Do not split at 0.
----------------
Together with the comment, I would invert this condition:

  if (SplitPoint == 0)
    return BreakableToken::Split(StringRef::npos, 0);
  return BreakableToken::Split(SplitPoint, 0);

I think you don't even need the comment then ..


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

BRANCH
  breakable-tokens

ARCANIST PROJECT
  clang