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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
