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

[Nick Desaulniers via Phabricator via llvm-commits]

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