This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix consecutive alignments in #else blocks
ClosedPublic

Authored by owenpan on May 6 2023, 6:50 PM.

Details

Summary

Since 3.8 or earlier, clang-format has been lumping all #else, #elif, etc blocks together when doing whitespace replacements and causing consecutive alignments across #else blocks.

Commit c077975 partially addressed the problem but also triggered "regressions".

This patch fixes the root cause of the problem and "reverts" c077975 (except for the unit tests).

Fixes https://github.com/llvm/llvm-project/issues/36070.
Fixes https://github.com/llvm/llvm-project/issues/55265.
Fixes https://github.com/llvm/llvm-project/issues/60721.
Fixes https://github.com/llvm/llvm-project/issues/61498.

Diff Detail

Event Timeline

owenpan created this revision.May 6 2023, 6:50 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMay 6 2023, 6:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel added inline comments.May 6 2023, 7:36 PM
clang/unittests/Format/FormatTestComments.cpp
2762

Should probably get rid of this comment?

owenpan added inline comments.May 6 2023, 8:21 PM
clang/unittests/Format/FormatTestComments.cpp
2762

Yep! I'll do it before landing. Thanks for catching it.

mitchell-stellar accepted this revision.May 7 2023, 7:23 AM

LGTM. Well done!

This revision is now accepted and ready to land.May 7 2023, 7:23 AM

I have to say, I don't understand it, but I believe you. Why is continuing, when the token is finalized the right thing?

clang/unittests/Format/FormatTestComments.cpp
4319

Unrelated?

owenpan added inline comments.May 7 2023, 2:21 PM
clang/unittests/Format/FormatTest.cpp
6371–6380

clang-format breaks the above into two separate sets:

#if FOO
int a = 1;
#else
#endif
#if BAR
int abc = 3;
#else
#endif

and:

#if FOO
#else
int ab = 2;
#endif
#if BAR
#else
int abcd = 4;
#endif

After it finishes with the first set, the preprocessor directives are marked as Finalized. Then, while the second set is being processed, the directives should not be skipped when tokens are added to the Change set. Otherwise, we would end up with the Change set below without any "scope" context:

int ab = 2;
int abcd = 4;
clang/unittests/Format/FormatTestComments.cpp
4319

My editor strips trailing whitespaces on save. I'll leave the fix in because it's not worth doing it in a separate patch.

I have to say, I don't understand it, but I believe you. Why is continuing, when the token is finalized the right thing?

See D150057#inline-1449546.

owenpan added inline comments.May 8 2023, 7:42 PM
clang/unittests/Format/FormatTest.cpp
6394
This revision was automatically updated to reflect the committed changes.
owenpan marked an inline comment as done.May 8 2023, 7:47 PM
clang/unittests/Format/FormatTest.cpp
6371–6380

Fascinating. But wouldn't the better fix be that the directives are not marked as finalized?

clang/unittests/Format/FormatTestComments.cpp
4319

Okay, but that wasn't really trailing, it was part of a string.

owenpan marked 2 inline comments as done.May 9 2023, 2:52 PM
owenpan added inline comments.
clang/unittests/Format/FormatTest.cpp
6371–6380

I tried that first, but it broke a lot of unit tests. I'll give it another try when I have time. :)

clang/unittests/Format/FormatTestComments.cpp
4319

Yeah. Fixed in e9acf00. Thanks!

owenpan marked 2 inline comments as done.Jun 21 2023, 3:21 PM
owenpan added inline comments.
clang/unittests/Format/FormatTest.cpp
6371–6380

See D153243.