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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:26
+
+AST_MATCHER(clang::CXXRecordDecl, hasNonStaticMethod) {
+  return hasMethod(unless(isStaticStorageClass()))
----------------
I think this and the next matcher can be a normal variable.

```
auto HasNonStaticMethod = hasMethod(unless(isStaticStorageClass()));

...
... allOf(anyOf(isStruct(), isClass()), HasMethods, HasNonStaticMethod)
...
```


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:55
+  // We may optionally ignore classes where all the member variables are public.
+  auto recordIsInteresting =
+      allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(),
----------------
Please make this variable uppercase, same for next matcher


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:76
+  const auto *Field = Result.Nodes.getNodeAs<FieldDecl>("field");
+  const auto *Record = Result.Nodes.getNodeAs<CXXRecordDecl>("record");
+  diag(Field->getLocation(),
----------------
Please add assertions that these pointers are not-null. Some random bug could match only one of these too and result in null-deref


================
Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h:31
+class NonPrivateMemberVariablesInClassesCheck : public ClangTidyCheck {
+  const bool IgnoreClassesWithAllMemberVariablesBeingPublic;
+  const bool IgnorePublicMemberVariables;
----------------
i believe usually the private state is written at the bottom of a class. I dont have a strong opinion, but i believe alex does prefer it.


================
Comment at: docs/ReleaseNotes.rst:99
+
+  Finds classes that not only contain the data (non-static member variables),
+  but also have logic (non-static member functions), and diagnoses all member
----------------
Please shorten the text in the release notes a bit. Usually one sentence should be enough, you can explain more in the docs


================
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}]}' --
----------------
I would prefer multiple test-files instead of mixing them all together.


================
Comment at: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp:9
+
+// Only data, do not warn
+
----------------
The test miss 
- templated classes (should be diagnosed normally) 
- inheritance (public, private, protected) (should not add a new warning in the subclass)
- and macros creating members. (dont know what to do there, but just warn?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52771