NoQ added inline comments.
================
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">;
----------------
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 :)
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
