aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Clang parts LGTM, thank you for this!
================
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:
> > 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]>;`.
I suspect you're correct. Perhaps the logic in ClangAttrEmitter.cpp is already
accounting for some oddity here that we're not. The way the generated mutual
exclusions look are:
```
if (const auto *Second = dyn_cast<AlwaysDestroyAttr>(A)) {
if (const auto *First = D->getAttr<NoDestroyAttr>()) {
S.Diag(First->getLocation(), diag::err_attributes_are_not_compatible) <<
First << Second;
S.Diag(Second->getLocation(), diag::note_conflicting_attribute);
return false;
}
return true;
}
```
Compared with your code, this is different. In your code, `CI` is analogous to
`Second` above, and `EA` is analogous to `First`. You diagnose at the location
of `First` but pass in `<< Second (CI) << First (EA)` as the arguments.
```
ErrorAttr *Sema::mergeErrorAttr(Decl *D, const AttributeCommonInfo &CI,
StringRef NewUserDiagnostic) {
if (const auto *EA = D->getAttr<ErrorAttr>()) {
std::string NewAttr = CI.getNormalizedFullName();
assert((NewAttr == "error" || NewAttr == "warning") &&
"unexpected normalized full name");
bool Match = (EA->isError() && NewAttr == "error") ||
(EA->isWarning() && NewAttr == "warning");
if (!Match) {
Diag(EA->getLocation(), diag::err_attributes_are_not_compatible)
<< CI << EA;
Diag(CI.getLoc(), diag::note_conflicting_attribute);
return nullptr;
}
if (EA->getUserDiagnostic() != NewUserDiagnostic) {
Diag(CI.getLoc(), diag::warn_duplicate_attribute) << EA;
Diag(EA->getLoc(), diag::note_previous_attribute);
}
D->dropAttr<ErrorAttr>();
}
return ::new (Context) ErrorAttr(Context, CI, NewUserDiagnostic);
}
```
Regardless, I'm content with the behavior of your tests, so I think we can move
on.
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
