[PATCH] D46519: [clang-format] Respect BreakBeforeClosingBrace while calculating length

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir marked an inline comment as done and an inline comment as not done.
krasimir added inline comments.


================
Comment at: lib/Format/ContinuationIndenter.cpp:44
+  int MatchingStackIndex = Stack.size() - 1;
+  while (MatchingStackIndex >= 0 && Stack[MatchingStackIndex].Tok != &Tok)
+    --MatchingStackIndex;
----------------
djasper wrote:
> I think this needs a long explanatory comment, possibly with an example of the problem it is actually solving.
> 
> Also, I am somewhat skeptical as we are using this logic for all paren kinds, not just braces. That seems to be unnecessary work and also might be unexpected at some point (although I cannot come up with a test case where that would be wrong).
Thanks! Added an example and tweaked it a little bit so that it doesn't traverse the stack unless it is visiting `tok::r_brace`.


================
Comment at: lib/Format/ContinuationIndenter.cpp:49
+      break;
+    while (MatchingStackIndex >= 0 &&
+           Stack[MatchingStackIndex].Tok != End->Next->MatchingParen)
----------------
djasper wrote:
> Maybe pull out a lambda:
> 
>   auto FindParenState = [&](const FormatToken *Tok) {
>     while (MatchingStackIndex >= 0 && Stack[MatchingStackIndex].Tok != &Tok)
>       --MatchingStackIndex;
>     return MatchingStackIndex >= 0 ? &Stack[MatchingStackIndex] : nullptr;
>   };
> 
> Then you could write the rest as:
> 
>   ...
>   FindParenState(&Tok);
>   const auto* End = Tok.MatchingParen
>   for (; End->Next; End = End->Next) {
>     if (End->Next->CanBreakBefore || !End->Next->closesScope())
>       break;
>     auto ParenState = FindParenState(End->Next->MatchingParen);
>     if (ParenState && ParenState.BreakBeforeClosingBrace)
>       break;
>   }
>   return End->TotalLength - Tok.TotalLength + 1;
> 
> 
> (not entirely sure it's better)
Yeah, a lambda is good.


Repository:
  rC Clang

https://reviews.llvm.org/D46519