This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix DefinitionBlockSeparator extra empty lines
ClosedPublic

Authored by ksyx on Feb 5 2022, 6:29 PM.

Details

Summary
  • Add or remove empty lines surrounding unions
  • Fixes https://github.com/llvm/llvm-project/issues/53229, in which keywords like class and struct in a line ending with left brace or whose next line is left brace only, will be falsely recognized as definition line, causing extra empty lines inserted surrounding blocks with no need to be formatted.

Diff Detail

Event Timeline

ksyx requested review of this revision.Feb 5 2022, 6:29 PM
ksyx created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2022, 6:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius requested changes to this revision.Feb 6 2022, 2:13 AM

Looks ok but please rework the tests to keep previous cases untouched.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
314

I'd rather see this added in another function e.g. bar3, because otherwise you don't test what was tested before.

This revision now requires changes to proceed.Feb 6 2022, 2:13 AM
clang/lib/Format/DefinitionBlockSeparator.cpp
39

I would prefer a switch over the tokens.

But definitely no else after return.

50

What about union?

ksyx updated this revision to Diff 406245.Feb 6 2022, 6:49 AM
ksyx marked 3 inline comments as done.
ksyx edited the summary of this revision. (Show Details)
  • Add or remove empty lines surrounding unions.
  • Move new test location.
  • Remove else immediately above return.
clang/lib/Format/DefinitionBlockSeparator.cpp
41

This is also an else after return. ;)

ksyx updated this revision to Diff 406271.EditedFeb 6 2022, 12:19 PM
  • Remove else after return
ksyx marked an inline comment as done.Feb 6 2022, 12:19 PM
owenpan added a subscriber: owenpan.Feb 6 2022, 5:48 PM
owenpan added inline comments.
clang/lib/Format/DefinitionBlockSeparator.cpp
49–63

Please use a for loop instead:

for (const FormatToken *CurrentToken = Line->First; CurrentToken;
     CurrentToken = CurrentToken->Next) {
  ...;
}
117–128

Use a for loop with the const FormatToken *CurrentToken loop variable similar to the above.

ksyx updated this revision to Diff 406309.Feb 6 2022, 5:59 PM

Use for loop to go through tokens in a line.

ksyx marked 2 inline comments as done.Feb 6 2022, 6:00 PM
curdeius accepted this revision.Feb 6 2022, 11:49 PM

LGTM.

clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
314

When I wrote this comment I thought that bar3 didn't exist... But well, I think I'll create a patch to clean up these tests a bit anyway.

This revision is now accepted and ready to land.Feb 6 2022, 11:49 PM
owenpan accepted this revision.Feb 6 2022, 11:50 PM
MyDeveloperDay accepted this revision.Feb 7 2022, 2:05 AM
ksyx marked an inline comment as done.Feb 7 2022, 6:10 AM
ksyx added inline comments.
clang/unittests/Format/DefinitionBlockSeparatorTest.cpp
314

Thank you very much for this!

ksyx marked an inline comment as done.Feb 7 2022, 6:16 AM
This revision was automatically updated to reflect the committed changes.