aaron.ballman added a comment.
I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm
worried we will wind up with clang-tidy checks that leave the user in an
impossible situation where they need to make data members private and provide
trivial accessors for them. Do you have thoughts on how to avoid that? The C++
Core Guidelines seem silent on the matter -- this might be worth raising with
the authors.
The HIC++ standard has an exception that requires the data type to be in an
`extern "C"` context, but I don't see that reflected in the tests or code.
================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:22
+
+AST_MATCHER(clang::CXXRecordDecl, hasMethods) {
+ return std::distance(Node.method_begin(), Node.method_end()) != 0;
----------------
Can drop the `clang::` qualifiers -- you're in the `namespace clang` scope
already.
================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:56
+ auto RecordIsInteresting =
+ allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(),
+ IgnoreClassesWithAllMemberVariablesBeingPublic
----------------
Neither the C++ Core Guidelines nor HIC++ carve out an exception for union
types. I think you should talk to the guideline authors to see what they think.
Perhaps first try to diagnose unions the same as structs and classes and see
how the results look to you -- if they look bad, you can incorporate those as
examples when discussing with the authors.
================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:82
+ "member variable '%0' of %1 '%2' has %3 visibility")
+ << Field->getName() << Record->getKindName() << Record->getName()
+ << Field->getAccess();
----------------
Drop the single quotes above and just pass in `Field` and `Record` directly --
the diagnostic printer will do the right thing.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52771
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
