aaron.ballman 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}}
+
----------------
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.
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
