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

25–28

And change this to something like

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

68–71

Comment needs updating?

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

8–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.

25–28

Done.

68–71

Done.

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

8–9

OK, used "user-provided" instead.

[lebedev.ri]

LG other than two nits, thank you!

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

25

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

9

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

9

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

9

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

9

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;
 }
 
 AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
-  return hasMethod(unless(isStaticStorageClass()))
+  return hasMethod(unless(anyOf(isStaticStorageClass(), isImplicit())))
       .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.
+  // member variables), but also have some logic (non-static, non-implicit
+  // 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
-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.
+Finds classes that contain non-static data members in addition to user-provided
+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;