philnik added a comment. In D157572#4617504 <https://reviews.llvm.org/D157572#4617504>, @aaron.ballman wrote:
> In D157572#4614561 <https://reviews.llvm.org/D157572#4614561>, @cjdb wrote: > >> In D157572#4613622 <https://reviews.llvm.org/D157572#4613622>, >> @aaron.ballman wrote: >> >>> In D157572#4612141 <https://reviews.llvm.org/D157572#4612141>, @cjdb wrote: >>> >>>> I don't dislike it, but I am a bit concerned about misuse being noisy. >>> >>> So you're concerned that a library author uses `diagnose_if` to add a >>> diagnostic to a warning group that makes the diagnostic seem too chatty, so >>> the user disables the group and loses the compiler's diagnostics? Or are >>> there other kinds of misuse you're worried about? >> >> More or less the former. I don't know if it'll actually manifest in practice >> though, I don't see many `diagnose_if` diagnostics to begin with. > > I think the former is sort of a "doctor, doctor, it hurts" situation; the > issue isn't really with `diagnose_if`, it's with the library author's use of > it. But at the same time, I can see why it would be beneficial to be able to > work around it if needed. > >>>> As much as I hate suppressing diagnostics, I think there needs to be a way >>>> to suppress the `diagnose_if` forms of warning without suppressing >>>> something that the compiler would otherwise generate. Something like: >>>> >>>> - `-Wno-deprecated`: suppresses anything that `-Wdeprecated` would turn on. >>>> - `-Wno-deprecated=diagnose_if`: just the ones flagged by `diagnose_if`. >>>> - `-Wno-deprecated=non-diagnose_if`: complement to #2. >>>> >>>> (and similarly for `-Wno-error=`.) >>>> >>>> I'm not sure about the system header knob though: `[[deprecated]]` and >>>> `[[nodiscard]]` still show up even when the declaration is in a system >>>> header? >>> >>> Correct, those will still show up when the declaration is in a system >>> header but not when the use is in a system header: >>> https://godbolt.org/z/PjqKbGsrr >> >> Right, my question was more "what is this knob doing?" > > Ah! We have various knobs on our diagnostics, like: > `ShowInSystemHeader` -- > https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L7818 > (if used, the diagnostic will be shown in a system header) > `SFINAEFailure` -- > https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L93 > (if used, the diagnostic causes SFINAE to fail in a SFINAE context) > `DefaultIgnore` -- > https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L141 > (if used, the warning is off by default and must be explicitly enabled via > its group) > `DefaultError` -- > https://github.com/llvm/llvm-project/blob/2dc6281b98d07f43a64d0ef34405d9a12d59e8b6/clang/include/clang/Basic/DiagnosticSemaKinds.td#L262 > (if used, the warning defaults to being an error but users can disable the > error with `-Wno`) > (etc) > > All of these have been of use to us as implementers, so it seems likely that > these same knobs would be of use to library authors adding their own compiler > diagnostics, so perhaps we should consider that as part of the design? > >>> We currently have `-Wuser-defined-warnings` as the warning group for >>> `diagnose_if` warning diagnostics, so I wonder if it would make sense to >>> allow `-Wno-deprecated` suppresses anything that `-Wdeprecated` would turn >>> on, while `-Wdeprecated -Wno-user-defined-warnings` would turn on only the >>> compiler-generated deprecation warnings and not the diagnose_if-generated >>> ones? >> >> Oh neat, this simplifies things a lot! > > I think it could make for a reasonable user experience. > > I'm curious what @erichkeane thinks as attributes code owner, but personally, > I like the idea of extending `diagnose_if` over the idea of adding > `clang::library_extension` because it's a more general solution that I think > will give more utility to our users. However, I also want to know if @philnik > @Mordante @ldionne (and others) share that preference because I think they're > going to be the guinea pigs^W^Wearly adopters of this functionality. I think this approach makes a lot of sense. I'm not sure all the knobs make sense to expose (e.g. `SFINAEFailure` could just be implemented with a normal `enable_if` IIUC), but at least `ShowInSystemHeader` and `DefaultIgnore` are ones where I already have use-cases in mind. As said before, we could also move our `atomic` ordering warnings from `-Wuser-defined-warnings` to `-Watomic-memory-ordering`, which seems like a huge usability improvement. I'm sure there will be more use-cases in the future for extending warning flags and having the ability to tune how and when warnings are emitted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157572/new/ https://reviews.llvm.org/D157572 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
