Page MenuHomePhabricator

[clang-format] Correctly indent closing brace of compound requires
ClosedPublic

Authored by rymiel on Sep 25 2022, 11:20 PM.

Details

Summary

When a compound requirement is too long to fit onto a single line, the
braces are split apart onto separate lines, and the contained expression
is indented. However, this indentation would also apply to the closing
brace and the trailing return type requirement thereof.
This was because the indentation level was being restored after all
trailing things were already read

With this change, the initial level of the opening brace is set before
attempting to read any trailing return type requirements

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

Diff Detail

Event Timeline

rymiel created this revision.Sep 25 2022, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 11:20 PM
rymiel requested review of this revision.Sep 25 2022, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 11:20 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel added a project: Restricted Project.Sep 25 2022, 11:20 PM

Note: I don't have the historical insight to know why the code order was as it was. I simply tried yoinking the place where the level was being set to occur earlier, and it surprisingly seemed to work, and also seemed to not break any other tests; I hope it doesn't break something in some subtle way

Hmm…it’s nice when it’s this simple, but I get an uneasy feeling about the knock on effects. I’d say go for it but perhaps we could run it over a larger code base to see the implications

I built 2 versions of clang-format, before and after this patch, and made 2 separate clones of all of LLVM+clang and ran them through either, removing all .clang-formats, then ran a recursive diff.

I found 1 difference, in a test case at clang/test/CodeGen/constrained-math-builtins.c and reduced it to the following:

clang-format before the patch produced this:

int foo() { ; };

  // Comment

#preprocessor

clang-format after this patch produced this:

int foo() { ; };

// Comment

#preprocessor

Very specific requirements here, needing a comment which follows a function with a non-empty body and with a (redundant) trailing semicolon, followed by any preprocessor directive.

Nevertheless, the output seems to make more sense *with* the patch, so I think this counts as a bug fix rather than a regression?

My testing surely isn't perfect but this was the only difference I found (besides instances of the specific bug that this patch was intended to fix)

This revision is now accepted and ready to land.Sep 27 2022, 10:42 PM
owenpan accepted this revision.Sep 28 2022, 1:26 PM
This revision was landed with ongoing or failed builds.Sep 30 2022, 10:35 PM
This revision was automatically updated to reflect the committed changes.