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:
> > 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.
================
Comment at: clang/test/Sema/attr-enforce-tcb-errors.cpp:23
+void both_tcb_and_tcb_leaf_on_separate_redeclarations();
+__attribute__((enforce_tcb_leaf("x"))) // FIXME: This should be an error.
+void both_tcb_and_tcb_leaf_on_separate_redeclarations() {}
----------------
aaron.ballman wrote:
> NoQ wrote:
> > Unfortunately the new facility doesn't catch this case because in
> > `handleEnforceTCBAttr()` the function doesn't yet know that it's a
> > redeclaration. I think this case is more important to catch than the
> > straightforward case (because it's very easy to make this mistake), so i'll
> > try to find a better place to emit the error. Is it ok if this goes into a
> > follow-up patch?
> I was about to comment on the code in SemaDeclAttr.cpp but skipped down here
> to see if you had a redeclaration test first, and found this comment instead.
> :-D I think you need to follow the `mergeFooAttr` pattern to enforce this
> properly (and I think it probably should be done in the initial patch, but it
> shouldn't be too painful hopefully). The basic idea is that you change
> `mergeDeclAttribute()` in SemaDecl.cpp to call `S.mergeEnforceTCBAttr()` that
> checks whether there are conflicting attributes or not. Then in
> `handleEnforceTCBAttr()` in SemaDeclAttr.cpp, you can call
> `mergeEnforceTCBAttr()` to handle the straightforward case.
Found it, thanks!!
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
