This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Respect BreakBeforeClosingBrace while calculating length
ClosedPublic

Authored by krasimir on May 7 2018, 4:26 AM.

Details

Summary

This patch makes getLengthToMatchingParen respect the BreakBeforeClosingBrace
ParenState for matching scope closers. In order to distinguish between paren states
introduced by real vs. fake parens, I've added the token opening the ParensState
to that struct.

Diff Detail

Event Timeline

krasimir created this revision.May 7 2018, 4:26 AM
djasper added inline comments.May 8 2018, 2:20 AM
lib/Format/ContinuationIndenter.cpp
81

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).

86

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)

lib/Format/ContinuationIndenter.h
215

The comment should include that this is not considered for memoization as the same state will always be represented by the same token (similar to other below). I wonder whether we should actually move the fields that don't affect memoization out to a different structure at some point.

krasimir updated this revision to Diff 145661.May 8 2018, 3:34 AM
krasimir marked 2 inline comments as done.
  • Address review comments
krasimir marked an inline comment as done and an inline comment as not done.May 8 2018, 3:35 AM
krasimir added inline comments.
lib/Format/ContinuationIndenter.cpp
81

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.

86

Yeah, a lambda is good.

krasimir marked an inline comment as done and an inline comment as not done.May 8 2018, 3:35 AM
krasimir marked 3 inline comments as done.
djasper accepted this revision.May 8 2018, 3:41 AM

Generally looks good.

lib/Format/ContinuationIndenter.cpp
95

I r_brace enough? Do we have something similar for TT_ArrayInitializer? I'd look at usages of BreakBeforeClosingBrace to determine.

This revision is now accepted and ready to land.May 8 2018, 3:41 AM
krasimir updated this revision to Diff 145677.May 8 2018, 6:16 AM
  • Added other parens detection
krasimir marked an inline comment as done.May 8 2018, 6:16 AM
krasimir added inline comments.
lib/Format/ContinuationIndenter.cpp
95

Thanks! Based on:
https://github.com/llvm-mirror/clang/blob/bf81db4ab70b45a77fba0d9f7d43190f1ad91bb9/lib/Format/ContinuationIndenter.cpp#L804
I've added also cases for array [-s and text proto <-s and updated comments.

krasimir marked an inline comment as done.May 8 2018, 6:17 AM
This revision was automatically updated to reflect the committed changes.