Page MenuHomePhabricator

[clang-tidy] find/fix unneeded trailing semicolons in macros
Needs ReviewPublic

Authored by trixirt on Nov 19 2020, 5:38 AM.

Details

Summary

clang's -Wextra-semi-stmt shows a large number of warnings that
should be cleaned up in the linux kernel. clang's fixer removes
just the semicolon. This is not always the best solution because
it either causes other whitespace problems or there is a better
semicolon to remove.

This change introduces linuxkernel-extra-semi as a place to
collect special purpose fixers. Starting with two fixers set by
the option 'Fixer'

'Switch'
switch() {} ; <-- the ';' is not needed.

The fixer takes care of formatting issues of having an empty line
when the ';' is removed.

'TrailingMacro'
#define M(a) a++; <-- clang-tidy fixes problem here
int f() {

int v = 0;
M(v); <-- clang reports problem here
return v;

}

Removing the semicolon at the macro use will fail checkpatch,
removing at the macro definition will not.

Diff Detail

Event Timeline

trixirt created this revision.Nov 19 2020, 5:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2020, 5:38 AM
trixirt requested review of this revision.Nov 19 2020, 5:38 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/linuxkernel/MacroTrailingSemiCheck.cpp
31 ↗(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

99 ↗(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

113 ↗(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

njames93 added inline comments.Nov 19 2020, 8:09 AM
clang-tools-extra/clang-tidy/linuxkernel/MacroTrailingSemiCheck.cpp
15 ↗(On Diff #306381)

Any reason for including the c standard header?

102–105 ↗(On Diff #306381)

Could this be a range for loop.

106–111 ↗(On Diff #306381)

Could this be refactored using llvm::find_if

116 ↗(On Diff #306381)

No need for this to be a temporary

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c
5

Please specify the location where this warning is emitted, you can check other test files but the typical format is
// CHECK-MESSAGES: :[[@LINE-<Offset>]]:<Column>: warning: unneeded semicolon.

6

Don't need to check the FIX-IT note, however it is preferred if you check the fix-it was applied correctly.
// CHECK-FIXES will run FileCheck over the input file after clang-tidy has gone in and made changes then check for lines in the output.
I haven't read enough into where the fix is but an example would be:
// CHECK-FIXES: #define M(a) a++{{$}}
the {{$}} says regex for end of line

njames93 added inline comments.Nov 19 2020, 10:31 AM
clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp
23–24

This check hasn't landed, can you please make sure your patches are against a commit on trunk, otherwise it can't be applied.

aaron.ballman requested changes to this revision.Nov 19 2020, 12:49 PM

The request in D90180 was to not add a check for each kind of problematic semicolon situation but to instead make a single check that has configuration options for which semi colon situations to care about. The proposed changes here should be rolled into that unified check.

This revision now requires changes to proceed.Nov 19 2020, 12:49 PM
trixirt updated this revision to Diff 307602.Nov 25 2020, 6:32 AM

Addresses issues before refactoring.

trixirt marked 8 inline comments as done.Nov 25 2020, 6:34 AM
trixirt updated this revision to Diff 308214.Nov 29 2020, 8:40 AM
trixirt edited the summary of this revision. (Show Details)

Refactor to combine switch and trailing macro into one fixer

trixirt updated this revision to Diff 308963.Dec 2 2020, 7:24 AM

address precheckin issues

Hi @trixirt , thanks for the follow up. I think the lint feedback about qualifying auto with pointers where applicable would be nice to have. Please make those suggested changes.

Should https://reviews.llvm.org/D90180 be abandoned in favor of this patch?

Would you mind editing the description/commit message of this patch to describe to kernel developers how they're expected to run this clang tidy check such that the fixits are applied?

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
21

Can this enum be declared within the class scope of ExtraSemiCheck rather than outside of it?

trixirt updated this revision to Diff 309270.Dec 3 2020, 8:47 AM

move enum

How this is run in the kernel is a wip so adding it to the commit log is not very helpful.
Here is the lkml rfc
https://lkml.org/lkml/2020/11/21/190
This calling in the kernel needs needs to change because of the refactor.

The auto suggestions fail to build, so they are not helpful.

Yes eventually the first review should be dropped, i'll do that when this one lands.

aaron.ballman added inline comments.Dec 4 2020, 11:34 AM
clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp
49

Something that's not for you to solve, but for us to think about...

The AST matchers have ways to inspect parent and child relationships between AST nodes but no way to inspect sibling relationships. It's really unfortunate just how much effort you have to go through after the AST matcher fires to figure this out. It would be much cleaner if there was a way for you to do something like: compoundStmt(hasSiblings(switchStmt(), nullStmt().bind("null"))) to test for a SwitchStmt node immediately followed by a NullStmt node that appear inside a CompoundStmt. I feel like that would make these checks a lot easier for you to write.

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
22

I'm not familiar with the way your CI plans to use this check, but: would it make sense to treat these as a bitmask rather than mutually exclusive options? e.g., the CI can enable both "switch" and "trailing macro" in a single pass of a file rather than run the check twice with different options on the same file? The configuration could then take a delimited list of pass options to enable to set up this bitmask, and the == tests for the FixerKind would then become & tests.

37

You can drop the enum elaboration here.

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h
37

right, this is a difference between C and C++; for C the enum keyword would be required always; for C++ it's not, and generally is omitted. (In C++ was also don't generally use struct or class for function parameters, or local variables, though you could.)