[PATCH] Implement string literal breaking on unbreakable token sequences.

[Manuel Klimek]

================
Comment at: lib/Format/Format.cpp:756
@@ -756,1 +755,3 @@
+    return breakProtrudingToken(Current, State, DryRun,
+                                /*TrailingUnbreakableLength=*/ 0);
   }
----------------
Daniel Jasper wrote:
> This might be a good use case for a default parameter!?
Done.

================
Comment at: lib/Format/Format.cpp:763
@@ -762,1 +762,3 @@
+                                bool DryRun,
+                                unsigned TrailingUnbreakableLength) {
     llvm::OwningPtr<BreakableToken> Token;
----------------
Daniel Jasper wrote:
> consider 'UnbreakableTailLength'
Done.

================
Comment at: lib/Format/Format.cpp:788
@@ +787,3 @@
+      // If a token that we cannot breaks protrudes, it means we were unable to
+      // break a sequence of tokens due to illegal breaks between the tokens.
+      // Thus, we recursively search backwards to try to find a breakable token.
----------------
Alexander Kornienko wrote:
> illegal -> disallowed?
Done.

================
Comment at: lib/Format/Format.cpp:790
@@ +789,3 @@
+      // Thus, we recursively search backwards to try to find a breakable token.
+      if (State.Column <= getColumnLimit() ||
+          Current.CanBreakBefore || !Current.Parent)
----------------
Daniel Jasper wrote:
> To me, this if is not self-explanatory. The comment above basically describes the subsequent recursive call.
> 
> As for the details:
> - Why is the "State.Column <= getColumnLimit()" not the first thing breakProtrudingToken does?
> - If we can break before the current token, then we will find the same solution using a different path in analyzeSolutionSpace(). Shouldn't we then return a very high penalty here so this branch is not used?
> - If this token does not have a parent, doesn't that mean that we failed and should return a very high penalty (as opposed to 0)? Or is the penalty for the protruding characters added elsewhere?
> 
1. State.Column <= getColumnLimit() is more complex for multi-line comments (and in the future for multi line string literals), and is thus handled in the BreakableToken abstraction.

2+3. Updated the method comment after figuring out offline what the problem was.

================
Comment at: lib/Format/Format.cpp:835
@@ -819,1 +834,3 @@
     if (BreakInserted) {
+      // FIXME: Do we want to add TrailingUnbreakableLength here?
+      State.Column = PositionAfterLastLineInToken;
----------------
Daniel Jasper wrote:
> I'd give yes 2:1 odds :-)
> 
> Testcase might look like:
> 
>   #define A \
>     string s = "String with exactly 72 chars"; \
>     int i;
Done.

================
Comment at: unittests/Format/FormatTest.cpp:3935
@@ -3934,1 +3934,3 @@
 
+TEST_F(FormatTest, BreakStringLiteralsBeforeUnbreakableTokenSequence) {
+  EXPECT_EQ("someFunction(\"aaaaaaaaaa\"\n"
----------------
Daniel Jasper wrote:
> Just FYI, using trailing line comments might give you slightly longer tokens before which clang-format won't break.
Ack.


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

BRANCH
  break-strings-off-by-one

ARCANIST PROJECT
  clang