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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
