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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir marked 3 inline comments as done.
krasimir added inline comments.


================
Comment at: lib/Format/BreakableToken.h:55-56
+///   been reformatted, and
+/// - replaceWhitespaceBefore, for executing the reflow using a whitespace
+///   manager.
+///
----------------
klimek wrote:
> Shouldn't that be called insertBreakBefore for consistency then?
> 
> Also, perhaps we want to rename replaceWhitespace to cleanupWhitespace or compressWhitespace or something...
insertBreakBefore will not be a good name for this, because it doesn't insert breaks.
There are two use-cases for this:
1. The old one: adjusting indent in comments, for example:
```
// before
   // after
```
2. The new one: reflowing, for example
```
before:
  // line1
  // line2
after:
  // line1 line2
```
The break between line1 and line2 gets removed in replaceWhitespaceBefore(2).

I'll rename replaceWhitespace to compressWhitespace.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1158-1159
+        CommentPragmasRegex.match(Current.TokenText.substr(2)) ||
+        Current.TokenText.substr(2).ltrim().startswith("clang-format on") ||
+        Current.TokenText.substr(2).ltrim().startswith("clang-format off"))
       return addMultilineToken(Current, State);
----------------
klimek wrote:
> Generally, we shouldn't need those here, as those should be part of the Token.Finalized state.
Here the problem is that the comments /* clang-format on */ and /* clang-format off */ control the formatting of the tokens in between, but do not control their own formatting, that is they are not finalized themselves.
What happens is that if we rely solely on Token.Finalized and the ColumnLimit is small, say 20, then:
```
/* clang-format off */ 
```
gets broken into:
```
/* clang-format off
  */
```

Add comments about this.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1220
+        RemainingTokenColumns = RemainingTokenColumns + 1 - Split.second;
+        ReflowInProgress = true;
         if (!DryRun)
----------------
klimek wrote:
> Explain this part in a comment.
Realized that the place of this is not here; introduced a proper method BreakableToken::getLineLengthAfterCompres instead.


https://reviews.llvm.org/D28764