[clang-format] avoid breaking )( with BlockIndent

[gedare]

Summary

The BracketAlignmentStyle BAS_BlockIndent was forcing breaks before a
closing right parenthesis yielding strange-looking results in case of
code structures that have a left parens immediately following a right
parens ")(" such as is seen with indirect function calls via function
pointers and with type casting.

See GitHub issues #57250 and #58496.

[gedare]

This comment was removed by gedare.

[gedare]

Allow line breaks in a continuation with BlockIndent after )(

[gedare]

Add a fix to enable breaking for continuation after a )(

[gedare]

A test case from the GitHub Issue https://github.com/llvm/llvm-project/issues/57250 is attached here:

test.c363 BDownload

When run without these changes, it yields:
clang-format -style="{BasedOnStyle: LLVM, AlignAfterOpenBracket: BlockIndent}" test.c

int foo(void) {
  int x = (int
  )(100000 + 200000 + 100000 + 200000 + 100000 + 200000 + 100000 + 200000 +
    100000 + 200000 + 100000 + 200000 + 100000 + 200000 + 100000 + 200000 +
    100000 + 200000);
  int y = 100000 + 200000 + 100000 + 200000 + 100000 + 200000 + 100000 +
          200000 + 100000 + 200000 + 100000 + 200000 + 100000 + 200000 +
          100000 + 200000 + 100000 + 200000;
}

When run with these changes, it yields:
clang-format -style="{BasedOnStyle: LLVM, AlignAfterOpenBracket: BlockIndent}" test.c

int foo(void) {
  int x = (int)(100000 + 200000 + 100000 + 200000 + 100000 + 200000 + 100000 +
                200000 + 100000 + 200000 + 100000 + 200000 + 100000 + 200000 +
                100000 + 200000 + 100000 + 200000);
  int y = 100000 + 200000 + 100000 + 200000 + 100000 + 200000 + 100000 +
          200000 + 100000 + 200000 + 100000 + 200000 + 100000 + 200000 +
          100000 + 200000 + 100000 + 200000;
}

[gedare]

Another test case for function pointers is here:

tmp.c237 BDownload

Before applying these changes, it is:

clang-format -style="{BasedOnStyle: LLVM, AlignAfterOpenBracket: BlockIndent}" tmp.c
int foo(long l) { return l; }

int main() {
  int (*abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ012345679
  )(long) = foo;

  (*abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ012345679
  )(100000000000L);
}

After applying these changes, it is:

clang-format -style="{BasedOnStyle: LLVM, AlignAfterOpenBracket: BlockIndent}" tmp.c
int foo(long l) { return l; }

int main() {
  int (*abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ012345679)(
      long
  ) = foo;

  (*abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ012345679)(
      100000000000L
  );
}

The function pointer declaration is still a little strange-looking, but I believe it is more in line with the intent behind the BlockIndent style.

[rymiel]

This patch will probably need unit tests

clang/lib/Format/TokenAnnotator.cpp

5004–5011

Please follow the convention on comments as seen on surrounding lines (i.e. use proper sentences with capitalization and a full stop at the end)

[MyDeveloperDay]

Definitely needs tests

clang/lib/Format/ContinuationIndenter.cpp

719–721

this would likely have been added for a reason, did this not break any tests?

[gedare]

clang/lib/Format/ContinuationIndenter.cpp

719–721

This does break the tests using AlwaysBreak (Google Test), it reformats

-    size_t foo = (*(function))(
-        Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong, BarrrrrrrrrrrrLong,
-        FoooooooooLooooong);
+    size_t foo =
+        (*(function))(Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong,
+                      BarrrrrrrrrrrrLong, FoooooooooLooooong);

This is my bad, sending my first patch to llvm/clang and trying to figure out the code/commit/test/review cycle. I'll work on resolving this, and adding / checking that the BlockIndent unittest covers the new behavior properly.

[gedare]

Run tests and fix bad breaks. Fix comment style.

[gedare]

Address review comments

[HazardyKnusperkeks]

In D137762#3919092, @MyDeveloperDay wrote:

Definitely needs tests

Could you please add tests in the clang/unittests/Format/FormatTest.cpp file?

clang/lib/Format/TokenAnnotator.cpp

5006–5010

This is horrible to read.

Could you split this into multiple statements? Maybe with lambdas, I don't know.

But I have no intention to ever understand that condition.

[gedare]

clang/lib/Format/TokenAnnotator.cpp

5006–5010

Sure. I was just following the prevailing style in this code base. I'll refactor.

[gedare]

add unit tests for BlockIndent

[gedare]

Refactor horrible to read logic

[gedare]

• fixup typo in unit test

[gedare]

I've added unit tests and confirmed (1) the new tests fail on current main and (2) all unit tests pass with this revision applied.

[100%] Running lit suite /llvm-project/clang/test/Unit

Testing Time: 250.49s
  Skipped:    31
  Passed : 15931
[100%] Built target check-clang-unit

All pending comments have been addressed.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

5006–5010

Sure. I was just following the prevailing style in this code base. I'll refactor.

No problem. We have some of this occurrences.

clang/unittests/Format/FormatTest.cpp

7237–7243 ↗

(On Diff #474901)

I know you copy what is there, but could you use verifyFormat? You can port the other checks also, but are not obligated to.

[gedare]

• fixup: replace EXPECT_EQ with verifyFormat
• fixup: port other EXPECT_EQ to verifyFormat

[gedare]

• fixup: unit test formatting

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

7215 ↗

(On Diff #475165)

Sorry for the confusion, you can (and should?) keep the Input variable. There is a verifyFormat with 4 arguments. So that the input in the formatting doesn't have to be the expected.

[gedare]

• FormatTest: restore Input variable to one test

[gedare]

clang/unittests/Format/FormatTest.cpp

7215 ↗

(On Diff #475165)

Ok, I restored it in this function. I didn't add it to the other functions since they weren't using it already. The vast majority of the calls to verifyFormat are using the 3-argument version. It's unclear what is preferred.

[HazardyKnusperkeks]

Thanks for your patience.
Please wait for @MyDeveloperDay .

clang/unittests/Format/FormatTest.cpp

7215 ↗

(On Diff #475165)

That depends. Most of the times the version with one string is enough. It verifies that:

• The input code is stable.
• It messes up the input and formats it.

The version with 2 strings does:

• Checks if the expected input is stable.
• Checks if the input is formatted to the expected.

The latter should be useful, if you want to verify what happens with your defined line breaks, that should not be stable, because https://github.com/llvm/llvm-project/blob/e989b8bb5fb36abac6e8f82809f06144dd762113/clang/unittests/Format/FormatTestUtils.h#L25 messup removes most of them.

[owenpan]

@gedare thanks for changing EXPECT_EQ to verifyFormat, but IMO we should do that in another patch so that it would be easier (at least for me) to review the new tests and to make sure no existing tests have been changed by accident.

clang/lib/Format/TokenAnnotator.cpp

5002

Can we simply add the above and leave the original return statement on line 5009 unchanged?

[gedare]

clang/lib/Format/TokenAnnotator.cpp

5002

Yes this seems to work. I'll push a diff shortly along with reverting the EXPECT_EQ outside of my new tests.

[gedare]

• fixup: use simpler logic for handling rparens
• Revert "fixup: port other EXPECT_EQ to verifyFormat"

[gedare]

I have changed the code as recommended by @owenpan with new unit tests passing, and reverting the modifications to existing unit tests. Someone else may janitor the EXPECT_EQ blocks separately.

I will not be able to look at this again for ~2 weeks. with luck it is ready to review and land, if not I'll deal with any further requests in December. Thanks for all the feedback and guidance so far!

[owenpan]

Thanks!

[owenpan]

@gedare do you need us to commit this patch on your behalf (using "Gedare Bloom <[e-mail]>")?

[gedare]

In D137762#3969770, @owenpan wrote:

@gedare do you need us to commit this patch on your behalf (using "Gedare Bloom <[e-mail]>")?

Yes, please. I don't have any llvm permissions, this is my first patch set for llvm actually. Anything else I need to do?

clang/lib/Format/ContinuationIndenter.cpp

void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
   // break after the opening parenthesis. Don't break if it doesn't conserve
   // columns.
   if ((Style.AlignAfterOpenBracket == FormatStyle::BAS_AlwaysBreak ||
        Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent) &&
       (Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) ||
        (Previous.is(tok::l_brace) && Previous.isNot(BK_Block) &&
         Style.Cpp11BracedListStyle)) &&
       State.Column > getNewLineColumn(State) &&
-      (!Previous.Previous || !Previous.Previous->isOneOf(
-                                 tok::kw_for, tok::kw_while, tok::kw_switch)) &&
+      (!Previous.Previous ||
+       !Previous.Previous->isOneOf(TT_CastRParen, tok::kw_for, tok::kw_while,
+                                   tok::kw_switch)) &&
       // Don't do this for simple (no expressions) one-argument function calls
       // as that feels like needlessly wasting whitespace, e.g.:
       //
       //   caaaaaaaaaaaall(
       //       caaaaaaaaaaaall(
       //           caaaaaaaaaaaall(
       //               caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
       Current.FakeLParens.size() > 0 &&

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   if (Right.is(tok::r_brace))
     return Right.MatchingParen && Right.MatchingParen->is(BK_Block);
 
   // We only break before r_paren if we're in a block indented context.
   if (Right.is(tok::r_paren)) {
     if (Style.AlignAfterOpenBracket != FormatStyle::BAS_BlockIndent ||
         !Right.MatchingParen) {
       return false;
     }
     const FormatToken *Previous = Right.MatchingParen->Previous;
-    return !(Previous && (Previous->is(tok::kw_for) || Previous->isIf()));
+    // Avoid breaking when there is another parens immediately following
+    // a closing parens, such as in cast operators and indirect function calls.
+    return !((Previous && (Previous->is(tok::kw_for) || Previous->isIf())) ||
+             (Right.Next &&
+              (Right.Next->is(tok::l_paren) ||
+               (Right.Next->is(tok::r_paren) &&
+                (Right.NestingLevel == 0 || Previous->is(tok::star))))));
   }
 
   // Allow breaking after a trailing annotation, e.g. after a method
   // declaration.
   if (Left.is(TT_TrailingAnnotation)) {
     return !Right.isOneOf(tok::l_brace, tok::semi, tok::equal, tok::l_paren,
                           tok::less, tok::coloncolon);
   }