[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1028
+  unsigned Penalty =
+      handleEndOfLine(Current, State, DryRun, AllowBreak);
 
----------------
krasimir wrote:
> Why `handleEndOfLine`? Is it guaranteed that here we've reached the end of the line?
No, this is used to do things in case we have reached the end of the line, as that's hard to check. I also wanted a better name, but wasn't able to come up with one (and we do similar things in other places here, where checking a condition is equivalent to running through all the code to do the things necessary).


================
Comment at: lib/Format/ContinuationIndenter.cpp:1284
+  unsigned StartColumn = State.Column - Current.ColumnWidth;
+  auto Delimiter = *getRawStringDelimiter(Current.TokenText);
   // The text of a raw string is between the leading 'R"delimiter(' and the
----------------
krasimir wrote:
> At this point, we compute `getRawStringDelimiter` twice: once here, and once in `getRawStringStyle` in the caller side. Ideally, we'd only like to compute it once. That's why it was a parameter before.
I got that, but I think passing around the delimiter is super awkward, and computing it is cheap. If computing it is the problem we should store it in the FormatToken.


================
Comment at: lib/Format/ContinuationIndenter.cpp:1408
+std::unique_ptr<BreakableToken> ContinuationIndenter::createBreakableToken(
+    const FormatToken &Current, LineState &State, bool AllowBreak) {
   unsigned StartColumn = State.Column - Current.ColumnWidth;
----------------
krasimir wrote:
> If `AllowBreak == false`, in which cases will this return a non-`nullptr`?
Only for string literals, as AllowBreak is really about whether we can break in the token space, but for comments (or raw strings, but those are handled differently anyway) we break in the comment space.


================
Comment at: unittests/Format/FormatTest.cpp:6322
                "#include \"some long include\" // with a comment\n"
-               "#include \"some very long include paaaaaaaaaaaaaaaaaaaaaaath\"",
+               "#include \"some very long include path\"\n"
+               "#include <some/very/long/include/path>\n",
----------------
krasimir wrote:
> What happened to the old test line?
It was unnecessarily long. I found it weird that it went out of its way to add more a's over the line limit.


https://reviews.llvm.org/D39900