[clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods

[vmiklos]

Summary

Otherwise we don't warn on a struct containing a single public int, but
we warn on a struct containing a single public std::string, which is
inconsistent.

[JonasToth]

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

22

maybe return llvm::any_of(Node.methods(), [](const CXXMethodDecl /* dunno which type this would be */& M) { return !M->isImplicit(); });?

[lebedev.ri]

Some nits.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

21

let's keep this one as-is.

31–34

And change this to something like

AST_MATCHER(CXXRecordDecl, hasNonStaticNonImplicitMethod) {
  return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit())))
      .matches(Node, Finder, Builder);
}

74–76

Comment needs updating?

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

9

I don't like the wording. We are not looking for explicit methods,
we are looking for user-provided methods, as opposed to compiler-provided methods.

[vmiklos]

I also noticed I forgot to clang-format the testcase, done now.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

21

Done.

31–34

Done.

74–76

Done.

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

9

OK, used "user-provided" instead.

[lebedev.ri]

LG other than two nits, thank you!

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

31

Please do rename it though, from hasNonStaticMethod to hasNonStaticNonImplicitMethod or something.

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

38

Can you please duplicate this test and add one static method (into S1Implicit, i think?).

[riccibruno]

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

13

Just a small remark: What do you mean exactly by "user-provided" ?

The term "user-provided" is defined in 11.4.2 [dcl.fct.def.default]/5 as

A function is user-provided if it is user-declared and not explicitly
defaulted or deleted on its first declaration.

[lebedev.ri]

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

13

Yeah, i'm not sure what is the right word to use here,
suggestions welcomed.
The previous wording was confusing too, since non-implicit can be read as explicit specifier which is not what is meant.

[riccibruno]

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

13

A suggestion without looking at the rest of the patch:

It depends on whether you want to include explicitly defaulted/deleted
member functions. If yes, then use "user-declared" and otherwise
use "user-provided" ?

[lebedev.ri]

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

13

explicitly defaulted/deleted member functions.

Yes, i do not think those should be exempt, i.e. i think the current code is correct (more tests needed?)
So "user-declared" it is.

[vmiklos]

Sorry, forgot to use the magic line at the end of the commit message to auto-close this review. Done in r351686, anyhow.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 
 AST_MATCHER(CXXRecordDecl, hasMethods) {
-  return std::distance(Node.method_begin(), Node.method_end()) != 0;
+  for (const auto &Method : Node.methods()) {
+    if (Method->isImplicit())
+      continue;
+
+    return true;
+  }
+  return false;
 }
 
 AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
   return hasMethod(unless(isStaticStorageClass()))
       .matches(Node, Finder, Builder);
 }
 
 AST_MATCHER(CXXRecordDecl, hasNonPublicMemberVariable) {
   return cxxRecordDecl(has(fieldDecl(unless(isPublic()))))
       .matches(Node, Finder, Builder);
 }
 
 AST_POLYMORPHIC_MATCHER_P(boolean, AST_POLYMORPHIC_SUPPORTED_TYPES(Stmt, Decl),
                           bool, Boolean) {
void NonPrivateMemberVariablesInClassesCheck::registerMatchers(
 
   // There are three visibility types: public, protected, private.
   // If we are ok with public fields, then we only want to complain about
   // protected fields, else we want to complain about all non-private fields.
   // We can ignore public member variables in structs/classes, in unions.
   auto InterestingField = fieldDecl(
       IgnorePublicMemberVariables ? isProtected() : unless(isPrivate()));
 
   // We only want the records that not only contain the mutable data (non-static
   // member variables), but also have some logic (non-static member functions).
   // We may optionally ignore records where all the member variables are public.
   Finder->addMatcher(cxxRecordDecl(anyOf(isStruct(), isClass()), hasMethods(),
                                    hasNonStaticMethod(),
                                    unless(ShouldIgnoreRecord),
                                    forEach(InterestingField.bind("field")))
                          .bind("record"),
                      this);
 }
 

docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

 .. title:: clang-tidy - misc-non-private-member-variables-in-classes
 
 misc-non-private-member-variables-in-classes
 ============================================
 
 `cppcoreguidelines-non-private-member-variables-in-classes` redirects here
 as an alias for this check.
 
-Finds classes that contain non-static data members in addition to non-static
+Finds classes that contain non-static data members in addition to explicit 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.
 
 Options
 -------
 
 .. option:: IgnoreClassesWithAllMemberVariablesBeingPublic
 
    Allows to completely ignore classes if **all** the member variables in that
    class a declared with a ``public`` access specifier.
 
 .. option:: IgnorePublicMemberVariables
 
    Allows to ignore (not diagnose) **all** the member variables declared with
    a ``public`` access specifier.

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

 
 protected:
   int S1_v2;
 
 private:
   int S1_v3;
 };
 
+// Only data and implicit methods, do not warn
+
+class C {
+public:
+    C() {}
+    ~C() {}
+};
+
+struct S1Implicit {
+    C S1Implicit_v0;
+};
+
 //----------------------------------------------------------------------------//
 
 // All functions are static, do not warn.
 
 struct S2 {
   static void S2_m0();
   int S2_v0;