We have 498 warnings that are disabled by default. Since the Clang policy is to enable warnings by default nowadays, I’m wondering if there’s any appetite for us to start moving 10~20 of these into on-by-default per release?
1 Like
The warnings that are off by default are mostly off for good reasons (high false positive rate, or targeted to specific narrow use-cases). If there’s some specific warning you think should be on by default (or a subset of a specific warning), it can go through the same sort of review we’d do for a new warning.
Probably some of the warnings are things we wouldn’t add at all today, but going through and removing warnings seems disruptive without much benefit.
Okay, thanks.
These warnings are always being checked regardless of whether or not they’re emitted, so pushing some of them into Clang Tidy might be an opportunity to reduce the amount of work Clang needs to do. I don’t know how expensive those warnings are to check though.
1 Like
There’s definitely interest in enabling more diagnostics by default, but as @efriedma-quic points out, a lot of these are off by default for valid reason. If you can find ones that are off by default but meet our usual quality bar (or can be modified to meet the bar), please post a patch, we’d love to review it!
I suspect there are more ancient C practices that we could be diagnosing by default. We’ve moved the needle on implicit int, functions without prototypes, and implicit pointer conversions; my instinct is that there’s more we could do here.
Another option to consider along these same lines is whether any of the warnings should be moved to -Wextra instead of -Wall.
1 Like
DiagnosticsEngine::isIgnored is used in a bunch of places to not do the work required for a diagnostic which is not enabled so the only reason to move something off to clang tidy is that we think it’s too opinionated to not be useful as a warning in the compiler, performance should not be a concern.
2 Likes
This works equally as well (lots of folks enable that pair), especially if we can promote some of the ones in -Wall and -Wextra to on-by-default. Is there a way to easily work out which warnings aren’t enabled by -Wall or -Wextra, or would we need to add a mechanism to do this?
Aside: do we have criteria for what should be in -Wextra instead of -Wall? I’m inclined to put them into -Wall by default.
I mostly agree, but would point out that not all diagnostics are amenable to isIgnored(). Sometimes that’s used to avoid doing work to discover the diagnostic needs to be emitting, but it’s also used when we’ve already decided to emit a diagnostic but we know emitting a good diagnostic will take extra work that we want to avoid if the diagnostic is locally ignored via a pragma or whatever.
I’m not certain if there’s an easier way, but I work backwards from llvm-project/DiagnosticGroups.td at main · llvm/llvm-project · GitHub, which reminds me that there’s also -Wmost as a thing.
Historically, we’ve matched what GCC puts into -Wextra, so anything they’ve promoted is a pretty easy sell so long as our implementation quality meets the usual bar for on-by-default warnings.
That said, I don’t think we have a hard rule that we need -Wextra (or -Wall) to match GCC’s decisions. So long as the diagnostic has a warning group users can disable, they’ve got a way to handle compiler-specific differences that turn out to be frustrating. However, we have a pile of warnings with no diagnostic group: llvm-project/warning-flags.c at main · llvm/llvm-project · GitHub
How difficult would it be to add an option that tells folks which warnings are enabled/disabled?
--print-warning-state=
all prints the status of all warnings
enabled prints all enabled warnings
disabled prints all disabled warnings
--print-remark-state=
all prints the status of all remarks
enabled prints all enabled remarks
disabled prints all disabled remarks
We’ve got something that prints a list of all the warnings, so I don’t think this will be that much more difficult? It might also be useful in weening folks off of -Weverything if we can give them another way to know which warnings they’re not using.
Grouping those would be great for paying down tech debt. Since they don’t have a -W flag, I’m guessing that they’re already on by default?
I don’t think it’d be too hard – basically all of this is driven by tablegen anyway. Btw, I’ve got thoughts on an approach to ween folks off of -Weverything, so if you’re moving in that direction, we should chat.
Yup! Alternatively, if you find any that are off by default, they can be safely removed. 
For what it’s worth diagtool can tell you the effect of warning flags; e.g.
diagtool show-enabled -Wno-everything -Wextra-semi /tmp/t.c
W ext_extra_semi [-Wextra-semi]
W ext_extra_semi_cxx11 [-Wc++11-extra-semi]
R remark_sloc_usage [-Wsloc-usage]
W warn_cxx98_compat_top_level_semi [-Wc++98-compat-extra-semi]
W warn_extra_semi_after_mem_fn_def [-Wextra-semi]
1 Like
Thanks, TIL! 
This is great, thanks! Perhaps instead of adding the proposed flags to Clang, we should be considering diagtool show-disabled test.c?