[PATCH] D40310: Restructure how we break tokens.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1525
+  if (!DryRun)
+    Token->adaptStartOfLine(0, Whitespaces);
+
----------------
krasimir wrote:
> If we indent here, shouldn't that also change ContentStartColumn?
Nope, this will exactly adapt the start of the line to ContentStartColumn. ContentStartColumn is where the line thinks it wants to be, not where it is.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1557
         if (LineIndex < EndIndex - 1)
+          // The last line's penalty is handled in addNextStateToQueue().
           Penalty += Style.PenaltyExcessCharacter *
----------------
krasimir wrote:
> How does the last line's penalty get handled in addNextStateToQueue()?
By the State's Column being above the column limit.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1578
+            LineIndex, TailOffset + Split.first + Split.second, ColumnLimit,
+            ContentStartColumn + ToSplitColumns, CommentPragmasRegex);
+        // Compute the comlumns necessary to fit the next non-breakable sequence
----------------
krasimir wrote:
> krasimir wrote:
> > nit: `BreakableToken::Split NextSplit = Token->getSplit(...)`
> Hm, right now `ContentStartColumn + ToSplitColumns` points to the column where the character at offset `TailOffset + Split.first` is supposed to be. Then `NextSplit` is relative to the offset `TailOffset + Split.first + Split.second`, so IMO it shouldn't use `ContentStartColumn + ToSplitColumns` as a start column. I think that `ToSplitColumns` needs to be computed as follows:
> ```
> unsigned ToSplitColumns = Token->getRangeLength(LineIndex, TailOffset, Split.first + Split.second, ContentStartColumn);
> ```
> Also, a comment of the intended meaning of `ToSplitColumns` would be helpful.
Good catch, but the solution is differnet. Using ContentStartColumn + ToSplitColumns is incorrect, we need ContentStartColumn + ToSplitColumns + 1 (as Split.second just contains the whitespace, but we want to consider that whitespace compressed).

I tried to write a test for this, and convinced myself that while +1 is correct, it is currently impossible to change behavior based on the missing +1.


https://reviews.llvm.org/D40310