This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix alignment in #else preprocessor blocks
ClosedPublic

Authored by mitchell-stellar on Sep 16 2022, 6:43 AM.

Details

Summary

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

clang-format makes multiple passes when #if/#else preprocessor blocks are found. It will make one pass for normal code and code in the #if block, and then it will make another pass for just the code in #else blocks. This often results in invalid alignment inside the else blocks because they do not have any scope or indentAndNestingLevel context from their surrounding tokens/lines.

This patch remedies that by caching any initial indentAndNestingLevel from a second pass and not breaking/returning early when a scope change is detected.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 6:43 AM
mitchell-stellar requested review of this revision.Sep 16 2022, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 6:43 AM

Please reupload the diff with the context.

clang/unittests/Format/FormatTest.cpp
5785

There should be a verifyFormat variant fitting for you.

Added context and use verifyFormat().

Looks good, please wait for other reviewers.

clang/unittests/Format/FormatTest.cpp
5747

Why limit the columns?

This revision is now accepted and ready to land.Sep 17 2022, 6:12 AM

Should we test nested if/else/endif

Do not limit columns and added nested test case.

owenpan added a subscriber: rymiel.May 4 2023, 1:23 PM

I'll revert this patch due to the regression https://github.com/llvm/llvm-project/issues/61498 unless any of you (and @rymiel ) objects.

After reviewing the GitHub issue, it looks like the IndentAndNestingLevel is sticky over the entire scope for #else blocks, so the regression only occurs within #else blocks in the same scope. The bug I fixed affected alignment in all #else blocks, regardless of scope.

I believe that if you choose to revert this @owenpan, you will see more bad alignments throughout #else blocks in an entire file. If you keep the fix, you will see bad alignment in #else blocks within the current scope. Pick your poison I guess. I personally prefer better alignment in separate #else blocks over the entire file, which is what prompted my bugfix in the first place. On those grounds I object to reverting the fix.

I retract my objection. I forgot that from clang-format's perspective, #else blocks in separate functions have the same indent and nesting level, and thus the same "scope", so the alignment issues may persist there. You will see misalignment in #else blocks over an entire source file whether you keep or revert this fix (i.e. pick your poison.) I suppose based on precedent we keep the misalignments I wanted to fix since I introduced misalignments in other cases.