[PATCH] D90180: [clang-tidy] find/fix unneeded semicolon after switch

Wed Nov 4 14:47:10 PST 2020

nickdesaulniers added a comment.

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").

> 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.

> 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?

> 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?  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 ?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90180/new/

https://reviews.llvm.org/D90180