[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

[Aaron Ballman via Phabricator via cfe-commits]

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