aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you for this effort!
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11087
+// TCB warnings
+def err_tcb_conflicting_attributes : Error<
+ "attributes '%0(\"%2\")' and '%1(\"%2\")' are mutually exclusive">;
----------------
NoQ wrote:
> aaron.ballman wrote:
> > NoQ wrote:
> > > aaron.ballman wrote:
> > > > NoQ wrote:
> > > > > Do i understand correctly that while "unknown attribute" is a warning
> > > > > ("must be an attribute for some other compiler, let's ignore"),
> > > > > misuse of a known attribute is typically an error ("ok, whatever you
> > > > > meant here, i have an opinion about what this really means and i
> > > > > don't like it")?
> > > > It depends strongly on the attribute and the problem at hand. My usual
> > > > rule of thumb is: if the attribute is being ignored because the code
> > > > makes no sense, it's an error, but if the attribute can still be useful
> > > > (perhaps after some adjustment), then warn. e.g., putting a calling
> > > > convention attribute on a variable of type `int` makes no sense -- that
> > > > should probably err rather than warn and ignore because the user did
> > > > something baffling.
> > > >
> > > > I could go either way on this one. Giving an error and forcing the user
> > > > to say what they mean is appealing, but it would also be defensible to
> > > > warn about the conflicting attribute and ignore just that one
> > > > (retaining the original attribute). I would say leave it as an error
> > > > and we can downgrade to a warning later if there's feedback from
> > > > compelling real world use cases.
> > > > retaining the original attribute
> > >
> > > There's an interesting aspect of error recovery here (which i didn't
> > > address yet but added FIXMEs). We probably shouldn't emit additional
> > > warnings based on the attributes about which we've already reported a
> > > conflict error; that's just unnecessary clutter. That'd mean that we
> > > should always discard the non-leaf attribute and keep the leaf attribute
> > > (if we can at all discard attributes). Or keep both and repeat the
> > > conflict check during checking in order to suppress the warning.
> > > That'd mean that we should always discard the non-leaf attribute and keep
> > > the leaf attribute (if we can at all discard attributes). Or keep both
> > > and repeat the conflict check during checking in order to suppress the
> > > warning.
> >
> > You can do `TheDecl->dropAttr<EnforceTCB>();` to drop the non-leaf
> > attributes (or, if easy, it's better to not add the problematic attribute
> > in the first place). Would you like to do that work in this patch or as a
> > follow-up? (I'm fine either way.)
> Hmm, that's indeed an easy and reasonable solution. Ideally i should only
> drop attributes with the same argument but that's too far into the area of
> diminishing returns for me :)
Yeah, I don't think we need to go that far unless there's some real world use
cases demonstrating the need.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91898/new/
https://reviews.llvm.org/D91898
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
