This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] avoid breaking )( with BlockIndent
ClosedPublic

Authored by gedare on Nov 9 2022, 8:44 PM.

Details

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.

Diff Detail

Event Timeline

gedare created this revision.Nov 9 2022, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 8:44 PM
gedare requested review of this revision.Nov 9 2022, 8:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 8:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gedare edited projects, added Restricted Project, Restricted Project; removed Restricted Project.Nov 9 2022, 8:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2022, 8:46 PM
gedare added a comment.Nov 9 2022, 9:09 PM
This comment was removed by gedare.
gedare updated this revision to Diff 474429.Nov 9 2022, 9:26 PM

Allow line breaks in a continuation with BlockIndent after )(

gedare updated this revision to Diff 474430.Nov 9 2022, 9:28 PM

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

gedare added a comment.Nov 9 2022, 9:34 PM

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

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 added a comment.Nov 9 2022, 9:44 PM

Another test case for function pointers is here:

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 removed a project: Restricted Project.

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)

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?

MyDeveloperDay requested changes to this revision.Nov 10 2022, 2:46 AM
This revision now requires changes to proceed.Nov 10 2022, 2:46 AM
gedare added inline comments.Nov 10 2022, 9:56 PM
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 updated this revision to Diff 474812.Nov 11 2022, 11:18 AM

Run tests and fix bad breaks. Fix comment style.

gedare updated this revision to Diff 474814.Nov 11 2022, 11:20 AM

Address review comments

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 marked 2 inline comments as done.Nov 11 2022, 2:24 PM
gedare added inline comments.
clang/lib/Format/TokenAnnotator.cpp
5006–5010

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

gedare updated this revision to Diff 474876.Nov 11 2022, 3:30 PM

add unit tests for BlockIndent

gedare updated this revision to Diff 474881.Nov 11 2022, 3:58 PM

Refactor horrible to read logic

gedare updated this revision to Diff 474901.Nov 11 2022, 5:17 PM
  • fixup typo in unit test
gedare marked an inline comment as done.Nov 11 2022, 5:35 PM

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.

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 updated this revision to Diff 475164.Nov 14 2022, 8:49 AM
  • fixup: replace EXPECT_EQ with verifyFormat
  • fixup: port other EXPECT_EQ to verifyFormat
gedare updated this revision to Diff 475165.Nov 14 2022, 8:51 AM
  • fixup: unit test formatting
gedare marked an inline comment as done.Nov 14 2022, 8:53 AM
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 updated this revision to Diff 475313.Nov 14 2022, 5:25 PM
  • FormatTest: restore Input variable to one test
gedare added inline comments.Nov 14 2022, 5:30 PM
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.

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.

MyDeveloperDay accepted this revision.Nov 16 2022, 3:36 PM
This revision is now accepted and ready to land.Nov 16 2022, 3:36 PM
owenpan edited the summary of this revision. (Show Details)Nov 16 2022, 11:00 PM

@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 added inline comments.Nov 17 2022, 11:31 AM
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 updated this revision to Diff 476196.Nov 17 2022, 11:37 AM
  • fixup: use simpler logic for handling rparens
  • Revert "fixup: port other EXPECT_EQ to verifyFormat"

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 accepted this revision.Nov 17 2022, 12:08 PM

Thanks!

@gedare do you need us to commit this patch on your behalf (using "Gedare Bloom <gedare@rtems.org>")?

gedare added a comment.Dec 5 2022, 8:38 PM

@gedare do you need us to commit this patch on your behalf (using "Gedare Bloom <gedare@rtems.org>")?

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

This revision was landed with ongoing or failed builds.Dec 6 2022, 1:05 AM
This revision was automatically updated to reflect the committed changes.