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

[Nick Desaulniers via Phabricator via cfe-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:
> 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?"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030