This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix formatting of struct-like records followed by variable declaration.
ClosedPublic

Authored by curdeius on Feb 14 2022, 2:41 PM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/24781.
Fixes https://github.com/llvm/llvm-project/issues/38160.

This patch splits TT_RecordLBrace for classes/enums/structs/unions (and other records, e.g. interfaces) and uses the brace type to avoid the error-prone scanning for record token.

The mentioned bugs were provoked by the scanning being too limited (and so not considering const or constexpr, or other qualifiers, on an anonymous struct variable declaration).

Moreover, the proposed solution is more efficient as we parse tokens once only (scanning being parsing too).

Diff Detail

Event Timeline

curdeius requested review of this revision.Feb 14 2022, 2:41 PM
curdeius created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 2:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius updated this revision to Diff 408629.Feb 14 2022, 2:46 PM

Remove unused.

MyDeveloperDay accepted this revision.Feb 15 2022, 12:27 AM

LGTM, I really like the approach of us annotating more like this, it makes the token much easier to reason about when there is ambiguity. nice one!

clang/lib/Format/UnwrappedLineFormatter.cpp
739

wow these are equivalent? do we need to worry about trailing comments?

public class A { /* comment */
This revision is now accepted and ready to land.Feb 15 2022, 12:27 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
470

Why not Union?

curdeius added a project: Restricted Project.Feb 15 2022, 1:03 AM
curdeius edited the summary of this revision. (Show Details)
curdeius added inline comments.Feb 15 2022, 2:08 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
470

Well, it wasn't handled here before.
I haven't found a case that breaks with unions and didn't want to incorporate even more changes.
I'll be happy to do this later though, I'd like to understand first why below we need !Style.BraceWrapping.AfterClass (there is a dozen of failing tests if removed, also, it's equivalent to !Style.BraceWrapping.AfterStruct 😵).

Also, as you might guess, unions are handled in the else that handles functions, which is, yuck, not the best thing to put it mildly.

739

Yes, before we were doing a poor man's scan skipping some (but not all) keywords (hence the bugs) that could appear before a record.
That was error-prone and redundant w.r.t. the parsing done in UnwrappedLineParser.

Anyway, good catch, I'll test what happens with comments.

curdeius added inline comments.Feb 15 2022, 2:09 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
722

@MyDeveloperDay, probably we'll need to check LastNonComment similarly to what I did with FirstNonComment for loops.

curdeius added inline comments.Feb 15 2022, 9:27 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
739

@MyDeveloperDay, do we want this (probably more coherent):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added

or this (current behaviour):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */\n"
                     "};", AllowSimpleBracedStatements); // added

(adapted from TEST_F(FormatTest, FormatShortBracedStatements))

clang/lib/Format/UnwrappedLineFormatter.cpp
739

@MyDeveloperDay, do we want this (probably more coherent):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added

or this (current behaviour):

verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
verifyFormat("struct A { /* comment */\n"
                     "};", AllowSimpleBracedStatements); // added

(adapted from TEST_F(FormatTest, FormatShortBracedStatements))

Ideally I'd say depending on the input. It not dependent on that, the former.

curdeius marked 3 inline comments as done.Feb 16 2022, 1:21 PM
curdeius added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
739

So, I did some testing.
First of all, I'd rather keep the current behaviour, otherwise it'll be a breaking change (for those that have such a case).
We can always do it later if need be (but I doubt it's important).

do we need to worry about trailing comments?

We don't, in tryMergeSimpleBlock, such comments are already on the next line so Line.Last is correctly pointing to the opening brace {.
Anyway, going to land as is (+ a new test case).

This revision was automatically updated to reflect the committed changes.
curdeius marked an inline comment as done.