[PATCH] Don't break line comments with escaped newlines.

[Alexander Kornienko]

================
Comment at: lib/Format/Format.cpp:908
@@ +907,3 @@
+      // Don't break string literals with escaped newlines. We're not likely to
+      // get them correctly, so better don't spoil them.
+      if (Current.TokenText.find("\\\n") != StringRef::npos)
----------------
Daniel Jasper wrote:
> The second sentence does not add much value. How about:
> 
> "Don't break string literals with escaped newlines. As clang-format must not change the string's content, it is unlikely that we'll end up with a better format."
Sounds good.

================
Comment at: lib/Format/Format.cpp:921
@@ -916,1 +920,3 @@
                 Current.Previous->Type != TT_ImplicitStringLiteral)) {
+      // Don't break line comments with escaped newlines. These look like
+      // separate line comments, but in fact contain consecutive lines including
----------------
Daniel Jasper wrote:
> The second sentence is confusing. "These look like separate line comments, but in fact contain consecutive lines..". There is no real contradiction.
> 
> How about:
> "These look like separate line comments, but in fact contain a single line comment with multiple lines including leading whitespace and the '//' markers."?
I don't see much difference, but let it be your version.

================
Comment at: lib/Format/Format.cpp:925
@@ +924,3 @@
+      //
+      // TODO: If we want to handle them correctly, we'll need to adjust leading
+      // whitespace in consecutive lines when changing indentation of the first
----------------
Daniel Jasper wrote:
> s/TODO/FIXME/
Done.

================
Comment at: lib/Format/Format.cpp:935
@@ +934,3 @@
+            1;
+        State.Column = Column;
+        return (Column > getColumnLimit())
----------------
Daniel Jasper wrote:
> I am confused by this. Shouldn't State.Column be set to the remaining length of the comment after the last escaped newline?
There's always a line break after the comment, and we don't adjust leading whitespace in consecutive lines, so we don't need to care about anything after the first escaped newline. 

Here I set State.Column to the last column of the first line to ensure proper penalty added if we violate the column limit in the first line. This is what is checked in FormatTest.cpp:938.

================
Comment at: lib/Format/Format.cpp:937
@@ +936,3 @@
+        return (Column > getColumnLimit())
+                   ? Style.PenaltyExcessCharacter * (Column - getColumnLimit())
+                   : 0;
----------------
Daniel Jasper wrote:
> With this, you are adding the penalty for the first line (i.e. for the length of the comment up to the first escaped newline).
> 
> It seems to me that we should either add the penalty for each of the comment's lines (except the last one, which is added later) or for none of them. Otherwise, this is a half-way solution that is only correct for a few edge cases and might confuse people.
As I've noted in my previous comment, we don't touch lines after the first one, so we don't need to add penalty for them (it will be constant).

================
Comment at: unittests/Format/FormatTest.cpp:938
@@ +937,3 @@
+                   getLLVMStyleWithColumns(50)));
+  EXPECT_EQ("int\n"
+            "a; // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
----------------
Daniel Jasper wrote:
> Maybe add a FIXME as this is not generally desirable. I know that there is one in the code, but having that also in the test is also helpful. E.g. imagine you fix this some day (either directly or by accident doing another change). If you the find this comment on the breaking test, you know that you can just fix the test.
These two tests ensure that we calculate columns used by the comment's first line correctly. There's nothing to fix here.


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