[PATCH] D90180: [clang-tidy] find/fix unneeded semicolon after switch
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 5 05:01:44 PST 2020
aaron.ballman added a comment.
In D90180#2374839 <https://reviews.llvm.org/D90180#2374839>, @nickdesaulniers wrote:
> In D90180#2357247 <https://reviews.llvm.org/D90180#2357247>, @aaron.ballman wrote:
>
>> When you pass `-fix` to clang-tidy, it will apply fix-its from the compiler as well. See https://godbolt.org/z/7vzM4b and try adding a semicolon to the end of the `switch` statement and note how it's removed when recompilation happens. What's more, the frontend will generate fix-its on some compile errors as well, so passing `-fix` may generate more changes than just removing semicolons even if you disable all frontend warnings. So if I understand your use case properly, you'd want to disable all frontend warnings, run clang-tidy with just this check enabled and pass `-fix` to be able to automatically catch and fix *only* the case where there's a semicolon after a switch (and just sort of hope that fixits from compile errors are reasonable to look at). Is that any more practical of a scenario?
>
> Doesn't `-W` disable all warnings? If `-W -Wextra-semi-stmt` would only warn/produce fixits related to `-Wextra-semi-stmt ` that might be feasible, but it's not as easy to integrate into the Linux kernel's build system (parts of the tree enable additional warnings), as compared to our current clang-tidy integration (which "just works").
Thanks for the further explanation -- I was thinking of something along the lines of `-W -Wextra-semi-stmt` but wasn't aware of the build system particulars.
>> While we have a module for the Linux kernel and it's reasonable to argue that this check certainly applies there, I also think it's defensible to argue that this is a somewhat odd case that's better handled by an out-of-tree script rather than having the community carry around functionality that duplicates what the frontend already supports.
>
> I'm happy to take ownership of linuxkernel scoped clang-tidy checks, as the Linux kernel maintainer for LLVM support. Even after using this for a tree-wide change, it's still useful IMO for us to automate clang-tidy runs to prevent this pattern from recurring. And anything we can do to promote LLVM within the Linux kernel community, such as leveraging clang-tidy, is a huge win for both projects. And I wouldn't mind carrying checks for a brief period and removing them if we find they no longer provide value. Worst case, just having the patch up on phab lets future travels find what was used to make large sweeping changes and can be linked to from kernel commit messages.
Thank you for volunteering! Having a primary point of contact who knows Linux kernel development for this module would be really helpful given the specific domain it's in.
>> Btw, I'd say that the whitespace is not really something we'd consider an issue -- we generally expect the user to format their code changes after applying fix-its.
>
> Heh, clang-tidy is another tool we're...hoping to promote the use of more. That's it's own distinct battle.
>
> In D90180#2368604 <https://reviews.llvm.org/D90180#2368604>, @aaron.ballman wrote:
>
>> Given that we already have a module for Linux kernel-specific clang-tidy checks for this to live in, I can hold my nose on the fact that it's poorly replicating the frontend's work because other users won't be surprised by it (in theory). So I'll retract my disapproval; you have compelling rationale for why you want to do it this way.
>>
>> Since the plan is to produce multiple distinct checks for each extraneous semicolon situation, then I think this check should probably be renamed to something more generic and given options to control which distinct scenarios to check for.
>
> That's fair feedback. Maybe something to note that this is more stylistic and doesn't necessarily fix bugs?
I don't have strong opinions on stylistic vs not.
>> This will reduce the amount of compilation overhead for the clang-tidy project over time by not needing to introduce a new check (with new boilerplate) for each scenario but should hopefully still allow you to do what you need (with config files perhaps) in your CI. WDYT?
>
> I don't see how renaming the check changes "compilation overhead" or why we think "compilation overhead" of clang tidy is a concern in this case?
I meant that if we had distinct checks `linuxkernel-switch-semi`, `linuxkernel-for-loop-semi`, `linuxkernel-middle-of-nowhere-semi`, etc that each one of those checks would require their own header file, source file, test files, documentation, etc. whereas if we had a single check, we'd reduce that overhead by only having one header, one source, one documentation, etc using config options, which makes fetching or building clang-tidy go ever-so-slightly faster.
> Maybe clarifying what you would prefer to see the check called and whether it would be in the linuxkernel namespace of checks or something else would help, @aaron.ballman ?
I definitely think the check should live in the `linuxkernel` module. How about `linuxkernel-spurious-semi`, `linuxkernel-extra-semi`, `linuxkernel-remove-useless-semi-colons`, or anything that makes you happy and sounds similarly generic?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90180/new/
https://reviews.llvm.org/D90180
More information about the cfe-commits
mailing list