[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute

[Matthias Gehre via cfe-commits]

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:
https://github.com/isocpp/CppCoreGuidelines/issues/742
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
"type1-dont-use-reinterpret_cast".
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
"cppcoreguidelines-pro-type-reinterpret-cast".
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?)


https://reviews.llvm.org/D24886