[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute
Matthias Gehre via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 27 10:55:28 PDT 2016
mgehre added a comment.
Thank your very much for your comments!
Let me try to give me reasoning for those points:
1. But it's missing some pieces, like test cases
I though about how to test this, having no semantic meaning itself.
I could look at the AST dump, but it does not even show the
rules that were passed, only that a "SuppressAttr" exists. Would that be enough?
2. Also, I suspect we will want this attribute to also be written on types
I was thinking about a case were that was useful, and didn't find any.
Which of course doesn't mean that there is none. I will add this.
3. No new undocumented attributes, please.
I completely agree that it cannot be merged like this. This depends a bit
on how our discussion turns out: Will this be specific to C++ Core Guidelines, or
clang-tidy or both or none? Then, should the clang documentation mention clang-tidy?
(or does that violate layering?)
4. Should we diagnose if asked to suppress a diagnostic that we don't support?
I image that the users workflow would be like this: Run clang-tidy (e.g. by build server);
get a warning; add [[suppress]], run clang-tidy again; see that the warning is gone. I don't see a big
risk in not diagnosing a wrongly spelled suppression, because the user will notice right away
that the warning is not suppressed. There is not other implicit side-effect.
As an ad-don, diagnosing if the spelling is just slightly off seems like a bonus to me, but I hope
that this could be deferred to another patch.
5. I'd suggest asking the editors of the core guidelines what attribute namespace they'd like used.
I followed your advice and asked here:
I will post updates to that issue here.
6. I believe this attribute should be used to silence diagnostics for more than just the C++ Core Guidelines,
so I don't think it makes sense to let them dictate what attribute namespace should be used.
Maybe I wanted to much here. There are two conflicting goals:
1. Suppress C++ Core Guidelines rules in a vendor-neutral way
2. Suppress specific clang(-tidy) warnings
I'm getting the feeling that we cannot have both in the same attribute.
For example, the C++ Core Guidelines say that the "No reinterpret_cast" rules shall be suppressed either by
saying "type" (also suppresses all other type related rules) or by saying "type.1" or by saying
When we want to suppress other clang(-tidy) warnings, it would make sense from a usability point of view
to take the warning ids that clang(-tidy) outputs. For that particular C++ Core Guideline rule, it would be
So even if we had the same attribute name for both goals, the rule names would have to differ.
What are your opinions on this point? (Should I put this on the mailing list?)
More information about the cfe-commits