[PATCH] Split long lines in multi-line comments.

[Daniel Jasper]

================
Comment at: lib/Format/Format.cpp:253
@@ -215,8 +252,3 @@
 
-    for (size_t i = 0; i < LineStarts.size(); ++i) {
-      if (IndentDelta > 0)
-        Replaces.insert(tooling::Replacement(SourceMgr, LineStarts[i], 0,
-                                             std::string(IndentDelta, ' ')));
-      else if (IndentDelta < 0)
-        Replaces.insert(
-            tooling::Replacement(SourceMgr, LineStarts[i], -IndentDelta, ""));
+    // Split long lines in comments.
+    const StringRef Prefix = findCommentLinesPrefix(Lines);
----------------
I think we should split out at least one extra function out of this:

splitCommentLine()

I think this function could have a parameters for the line content, prefix, indent etc. and then does everything that applies locally for a single line of a comment. It will then probably be usable for both block comments and line comments.

================
Comment at: lib/Format/Format.cpp:225
@@ +224,3 @@
+    const SourceLocation TokenLoc = Tok.FormatTok.Tok.getLocation();
+    const int TokenIndent = SourceMgr.getSpellingColumnNumber(TokenLoc) - 1;
+    const int IndentDelta = Indent - TokenIndent;
----------------
nit: maybe s/TokenIndent/CurrentIndent/

================
Comment at: lib/Format/Format.cpp:276
@@ +275,3 @@
+        size_t SpacePos = Line.find_last_of(WhiteSpaceChars, ColumnLimit + 1);
+        if (SpacePos == StringRef::npos)  // Break at first possibility.
+          SpacePos = Line.find_first_of(WhiteSpaceChars);
----------------
This comment (because of its location) is a bit confusing. How about changing the comment in line 274 to: // Try to break at the last whitespace before the column limit
?

nit: One space before //

================
Comment at: lib/Format/Format.cpp:278
@@ +277,3 @@
+          SpacePos = Line.find_first_of(WhiteSpaceChars);
+        if (SpacePos == StringRef::npos)  // No whitespace, give up.
+          break;
----------------
Can this ever happen? How would it not have triggered the test in line 276?

================
Comment at: lib/Format/Format.cpp:280
@@ +279,3 @@
+          break;
+        StringRef FirstPiece = Line.substr(0, SpacePos).rtrim();
+        Line = Line.substr(SpacePos).ltrim();
----------------
maybe s/FirstPiece/BeforeSpace/

Does rtrim() actually change the behavior? I would have thought that line 277 already ensures finding the first space ...


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