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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
