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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:375-378
+def warn_changed_error_warning_attr_text : Warning<
+  "duplicate attribute changes text from %0 to %1">,
+  InGroup<DuplicateErrorWarningAttr>;
+
----------------
FWIW, elsewhere we typically use `warn_duplicate_attribute` and `note_previous_attribute` as a pair. It probably makes sense to move those to be common diagnostics instead of adding a new one with a new warning group.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
 
----------------
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.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:949-960
+static void handleErrorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+    return;
+  D->addAttr(::new (S.Context) ErrorAttr(S.Context, AL, Str));
+}
+static void handleWarningAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > Pretty sure you can get rid of the manual handlers and just use `SimpleHandler = 1` in Attr.td, unless we need to add extra diagnostic logic (which we might need to do for attribute merging).
> It seems that the use of `Args` in the tablegen definitions is incompatible with `SimpleHander`:
> ```
> In file included from /android0/llvm-project/clang/lib/Sema/ParsedAttr.cpp:108:
> tools/clang/include/clang/Sema/AttrParsedAttrImpl.inc:4039:32: error: no matching constructor for initialization of 'clang::ErrorAttr'
>   D->addAttr(::new (S.Context) ErrorAttr(S.Context, Attr));
>                                ^         ~~~~~~~~~~~~~~~
> tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
> class ErrorAttr : public Attr {
>       ^
> tools/clang/include/clang/AST/Attrs.inc:3601:7: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
> tools/clang/include/clang/AST/Attrs.inc:3624:3: note: candidate constructor not viable: requires 3 arguments, but 2 were provided
> ```
> where the 3rd argument would be the `llvm::StringRef UserDiagnostic`.
Oh yeah, derp, that's my mistake. Also, it's moot given that we want to check merging logic anyway (often the handler will call the merge function to generate the semantic attribute to add to the declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030