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

[Alexander Kornienko]

================
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;
----------------
Daniel Jasper wrote:
> nit: maybe s/TokenIndent/CurrentIndent/
Done.

================
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);
----------------
Daniel Jasper wrote:
> 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.
Done.

================
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);
----------------
Daniel Jasper wrote:
> 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 //
Done.

================
Comment at: lib/Format/Format.cpp:278
@@ +277,3 @@
+          SpacePos = Line.find_first_of(WhiteSpaceChars);
+        if (SpacePos == StringRef::npos)  // No whitespace, give up.
+          break;
----------------
Daniel Jasper wrote:
> Can this ever happen? How would it not have triggered the test in line 276?
I've changed the code to make it clear what happens.

================
Comment at: lib/Format/Format.cpp:280
@@ +279,3 @@
+          break;
+        StringRef FirstPiece = Line.substr(0, SpacePos).rtrim();
+        Line = Line.substr(SpacePos).ltrim();
----------------
Daniel Jasper wrote:
> maybe s/FirstPiece/BeforeSpace/
> 
> Does rtrim() actually change the behavior? I would have thought that line 277 already ensures finding the first space ...
I think FirstPiece is more suitable here.

Line 275 finds the last space before ColumnLimit, so we need to find the first one. In case line 277 gets executed, rtrim doesn't make a difference.


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