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

Tue Sep 27 11:24:35 PDT 2016

aaron.ballman added a comment.

In https://reviews.llvm.org/D24886#554130, @mgehre wrote:

> 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?

That's a good start. Other tests that are required: the attribute appertains to the proper syntactic constructs and is diagnosed otherwise, attribute has the correct number and kind of arguments, the arguments are sane, etc.

> 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.

If there are no use cases for it, then I guess we don't need to support it.

> 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?)

I agree, we want to make sure the docs reflect our intended design. I don't think it's a problem for the clang docs to mention clang-tidy. Certainly we have LLVM documentation that mentions clang.

> 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.

I think that's definitely a reasonable use case, but I don't think it's a compelling explanation of why we should not warn the user "I have no idea what you're talking about", either. The same could be said of many diagnostics we give -- the user will notice that their code doesn't work properly. However, I tend to be in the camp of "warn on suspicious activity" camp.

> 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.

Certainly!

> 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.

Thanks!

> 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:

> 7. Suppress C++ Core Guidelines rules in a vendor-neutral way

> 8. Suppress specific clang(-tidy) warnings I'm getting the feeling that we cannot have both in the same attribute.

I think we do our users a major disservice by not trying to do both with the same attribute. As a user, I do not want to have to remember which way to spell an attribute to silence warnings. This is especially important were we ever to allow triggering clang-tidy diagnostics through the clang frontend.

> 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?)

I guess I fail to see what the technical issue is (and perhaps I'm just obtuse), but I think that getting a wider audience is not a bad idea.

https://reviews.llvm.org/D24886