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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
