[PATCH] D106030: [Clang] add support for error+warning fn attrs

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > nickdesaulniers wrote:
> > > > aaron.ballman wrote:
> > > > > nickdesaulniers wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > GCC doesn't seem to support this spelling -- do they have a different one we should reuse?
> > > > > > I think these diagnostics don't have corresponding flags in GCC, so they cannot be disabled.
> > > > > > 
> > > > > > Without adding this, clang/test/Misc/warning-flags.c would fail, because I was adding a new unnamed warning `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding this line here, and doing something else?
> > > > > > 
> > > > > > That test says we shouldn't be adding new warnings not controlled by flags, but that's very much my intention.
> > > > > > 
> > > > > > Though now `-Wno-user-diagnostic` doesn't produce a `-Wunknown-warning-option` diagnostic...
> > > > > Ah! I think the warning attribute should be controllable via a diagnostic flag (we should always be able to disable warnings with some sort of flag) and I don't think the error attribute should be controllable (an error is an error and should stop translation, same as with `#error`).
> > > > > 
> > > > > Normally, I'd say "let's use the same flag that controls `#warning`" but...
> > > > > ```
> > > > > def PoundWarning : DiagGroup<"#warnings">;
> > > > > ```
> > > > > That's... not exactly an obvious flag for the warning attribute. So I would probably name it `warning-attributes` similar to how we have `deprecated-attributes` already.
> > > > Done, now `-Wno-warning-attributes` doesn't produce `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was there something else I needed to add?
> > > huh, that sounds suspicious -- you don't need to do anything special for `-Wno-foo` handling, that should be automatically supported via tablegen. I'm not certain why you're not seeing `-Wno-warning-attributes` silencing the warnings.
> > Ah! Because I was testing `__attribute__((error("")))` not `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the latter, not the former.  I should add a test for `-Wno-warning-attributes`! Is this something I also need to add documentation for?
> Given that this behavior surprised you, I certainly wouldn't oppose mentioning it in the documentation, but I also don't think it's strictly required. That said, I do agree about adding some additional test coverage for when the warning is disabled.
> 
> Just to double-check: do you think this functionality needs an "escape hatch" for the error case? e.g., should the `error` attribute generate a warning diagnostic that defaults to being an error, so we have `-Wno-warning-attributes` to turn off `warning` attribute diagnostics and `-Wno-error-attributes` to turn off `error` attribute diagnostics?
Ah, GCC also controls this via `-Wattribute-warning`; let me rename my implementation.

> do you think this functionality needs an "escape hatch" for the error case?

No.  If users don't want an error, then they should prefer `__attribute__((warning("")))` to `__attribute__((error("")))`.


================
Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1079
+public:
+  // TODO: make LocCookie optional
+  DiagnosticInfoDontCall(StringRef CalleeName, unsigned LocCookie)
----------------
nickdesaulniers wrote:
> TODO: play with llvm::Optional
Ok, that was fun, but I didn't find it made construction sites of `DiagnosticInfoDontCall` nicer.  This is also how `DiagnosticInfoInlineAsm` works, which is where I got the idea for `!srcloc` from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030