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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:56
+  auto RecordIsInteresting =
+      allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(),
+            IgnoreClassesWithAllMemberVariablesBeingPublic
----------------
aaron.ballman wrote:
> 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.
As discussed, let's just drop HICPP alias,
and CPPCG only talks about struct/class, not union.
https://github.com/isocpp/CppCoreGuidelines/issues/1281


================
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();
----------------
aaron.ballman wrote:
> Drop the single quotes above and just pass in `Field` and `Record` directly -- the diagnostic printer will do the right thing.
Nice, but that does not seem to dump `getKindName()`.


================
Comment at: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp:1
+// RUN: %check_clang_tidy -check-suffix=NONPRIVATE %s misc-non-private-member-variables-in-classes %t
+// RUN: %check_clang_tidy -check-suffix=NONPRIVATE %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 0}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 0}]}' --
----------------
zinovy.nis wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > JonasToth wrote:
> > > > I would prefer multiple test-files instead of mixing them all together.
> > > Hmm, no. Then the tests will have to be duplicated in N files,
> > > because i really do want to see what each of these 4 configurations do on the *same* input.
> > > 
> > > It only looks ugly because the script only supports `-check-suffix=`,
> > > and not `-check-suffixes=`, which would significantly cut down on check-lines.
> > Ok, if you prefer this.
> Not an issue anymore. You can use `-check-suffixes` since now.
Yay, look at this beauty :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52771