nickdesaulniers added inline comments.
================
Comment at: clang/test/Sema/attr-error.c:31-32
+
+__attribute__((error("foo"))) int bad5(void); // expected-error {{'error'
and 'warning' attributes are not compatible}}
+__attribute__((warning("foo"))) int bad5(void); // expected-note {{conflicting
attribute is here}}
+
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > aaron.ballman wrote:
> > > I think the diagnostic order is backwards here. The first declaration is
> > > where I'd expect the note and the second declaration is where I'd expect
> > > the error. (The idea is: the first declaration adds an attribute to the
> > > decl, so the redeclaration is what introduces the conflict and so that's
> > > where the error should live.) As an example:
> > > https://godbolt.org/z/bjGTWxYvh
> > I'm not sure how to reconcile this. Looking at `bad4` above (different case
> > than `bad5`), the diagnostic we get is:
> > ```
> > /tmp/y.c:1:30: error: 'warning' and 'error' attributes are not compatible
> > __attribute__((error("foo"), warning("foo")))
> > ^
> > /tmp/y.c:1:16: note: conflicting attribute is here
> > __attribute__((error("foo"), warning("foo")))
> > ^
> > 1 error generated.
> > ```
> > which looks correct to me. If I flip the order these are diagnosed in, that
> > fixes `bad5` but if I flip around the ordering:
> > ```
> > --- a/clang/lib/Sema/SemaDeclAttr.cpp
> > +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> > - Diag(CI.getLoc(), diag::err_attributes_are_not_compatible) << CI <<
> > EA;
> > - Diag(EA->getLocation(), diag::note_conflicting_attribute);
> > + Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
> > + << CI << EA;
> > + Diag(CI.getLoc(), diag::note_conflicting_attribute);
> > ```
> > Then `bad4` doesn't look quite correct.
> > ```
> > /tmp/y.c:1:16: error: 'warning' and 'error' attributes are not compatible
> > __attribute__((error("foo"), warning("foo")))
> > ^
> > /tmp/y.c:1:30: note: conflicting attribute is here
> > __attribute__((error("foo"), warning("foo")))
> > ^
> > ```
> > Perhaps in the callers of `handleErrorAttr` I'm confusing which `Decl` is
> > the "new" vs the "old?"
> > which looks correct to me.
>
> That also looks correct to me; the second attribute is the diagnostic and the
> first attribute is the note.
>
> > Perhaps in the callers of handleErrorAttr I'm confusing which Decl is the
> > "new" vs the "old?"
>
> Huh, this is rather strange. The logic you're using is basically the same as
> `checkAttrMutualExclusion()`, just with extra work to figure out which kind
> of attribute you're dealing with. I'm not seeing what's causing the order to
> be different when I reason my way through the code. Perhaps in the `bad4()`
> case, are we somehow processing the attributes in reverse order?
>
> FWIW, at the end of the day, we can live with the diagnostic in either
> situation, even if they're a bit strange. I think the merge behavior (two
> different decls) is more important to get right because that's going to be
> the far more common scenario to see the diagnostic in. If it turns out that
> it's a systemic issue (or just gnarly to figure out), it won't block this
> review. We can always improve the behavior in a follow-up.
I suspect that `DiagnoseMutualExclusions` (via tablegen) would be able to catch
this, had we distinct `Attr`s for warning and error; example tablegen rule:
`def : MutualExclusions<[Hot, Cold]>;`.
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
