[PATCH] D28764: [clang-format] Implement comment reflowing (v3)

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: lib/Format/BreakableToken.cpp:279-280
+  return Content.size() >= 2 &&
+      Content != "clang-format on" &&
+      Content != "clang-format off" &&
+      !Content.endswith("\\") &&
----------------
Can we now use delimitsRegion here?


================
Comment at: lib/Format/ContinuationIndenter.cpp:1090-1092
+// Checks if Token delimits formatting regions, like /* clang-format off */.
+// Token must be a comment.
+static bool delimitsRegion(const FormatToken &Token) {
----------------
delimitsRegion seems overly abstract. Perhaps switchesFormatting?


================
Comment at: lib/Format/ContinuationIndenter.cpp:1205-1210
+    if (SplitBefore.first != StringRef::npos) {
+      TailOffset = SplitBefore.first + SplitBefore.second;
+      ReflowInProgress = true;
+    } else {
+      ReflowInProgress = false;
+    }
----------------
krasimir wrote:
> klimek wrote:
> > (optional) I'd probably write this:
> >   ReflowInProgress = SplitBefore.first != StringRef::npos;
> >   if (ReflowInProgress) {
> >     TailOffset = SplitBefore.first + SplitBefore.second;
> >   }
> How about this?
That works.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1231-1233
+      unsigned RemainingTokenColumnsAfterCompress =
+          Token->getLineLengthAfterCompress(LineIndex, TailOffset,
+                                            RemainingTokenColumns, Split);
----------------
krasimir wrote:
> klimek wrote:
> > I think AfterCompression reads more naturally. Perhaps we should use "trim" instead of compress, now that I think about it.
> > So I'd probably go for getTrimmedLineLength or something.
> I don't agree with "trimmig" because i have a connotation of "trimming" around the ends of strings like in ltrim/rtrim and this fundamentally operates somewhere inside. +1 for AfterCompression, though.
Yea, correct, I completely missed that this might happen in the middle of the string if we happen to be the first token that runs over, which is a rather interesting case :)


================
Comment at: lib/Format/WhitespaceManager.cpp:131
+        Changes[i - 1].Kind == tok::comment &&
+        OriginalWhitespaceStart != PreviousOriginalWhitespaceEnd;
   }
----------------
krasimir wrote:
> klimek wrote:
> > Why was this needed? (what breaks without this change)
> This is a dirty hack. The problem is that WhitespaceManager does its own formatting for trailing comments and sometimes it re-formats a piece of line even after it's been reflown with the previous line. Consider if we reflow the second line up:
> ```
> // line 1
> // line 2
> ```
> That amounts to 2 changes, first (delimited by ()) for the whitespace between the two tokens, and second (delimited by []) for the beginning of the next line:
> ```
> // line 1(
> )[// ]line 2
> ``` 
> So in the end we have two changes like this:
> ```
> // line 1()[ ]line 2
> ```
> Note that the OriginalWhitespaceStart of the second change is the same as the PreviousOriginalWhitespaceEnd of the first change.
> In this case, the above code marks the second line as trailing comment and afterwards applies its own trailing-comment-alignment logic to it, which might introduce extra whitespace before "line 2".
> 
> For a proper solution we need a mechanism to say that a particular change emitted through the whitespace manager breaks the sequence of trailing comments.
> 
> 
Ok. In this case this needs a largish comment with a FIXME, like you just described ;)


https://reviews.llvm.org/D28764