[PATCH] D157572: [clang] Add `[[clang::library_extension]]` attribute

[Nikolas Klauser via Phabricator via cfe-commits]

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