[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.

In https://reviews.llvm.org/D52771#1263404, @lebedev.ri wrote:

> FIXME: `IgnoreClassesWithAllMemberVariablesBeingPublic` needs to be somehow enabled for cppcoreguidelines check.
>  I don't know if it is possible, and how.


IIRC, the idea is to override `getModuleOptions()` and specify different default configuration options for the alias.



================
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();
----------------
lebedev.ri wrote:
> 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()`.
Correct - it only works for automatically quoting `NamedDecl` objects


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:90
+  diag(Field->getLocation(), "member variable %0 of %1 %2 has %3 visibility")
+      << Field << Record->getKindName() << Record << Field->getAccess();
+}
----------------
I cannot think of a situation in which the class name is actually useful information to present to the user, so I'd recommend dropping it (and the kind name as well). However, if you have a specific case in mind where the class name is useful for understanding how to fix the issue, I'd love to see it.


================
Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9
+
+Finds classes that not only contain the data (non-static member variables),
+but also have logic (non-static member functions), and diagnoses all member
----------------
I'd reword this a bit to:
```
Finds classes that contain non-static data members in addition to non-static member functions and diagnose all data members declared with a non-``public`` access specifier. The data members should be declared as ``private`` and accessed through member functions instead of exposed to derived classes or class consumers.
```


================
Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:14-15
+
+Optionally, classes with all member variables being ``public`` could be
+ignored, and optionally all ``public`` member variables could be ignored.
+
----------------
I'd drop this paragraph entirely.


================
Comment at: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:28
+   Allows to ignore (not diagnose) **all** the member variables with ``public``
+   visibility scope.
----------------
with public visibility scope -> declared with a public access specifier


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52771