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