Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp =================================================================== --- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "../misc/NonPrivateMemberVariablesInClassesCheck.h" #include "../misc/UnconventionalAssignOperatorCheck.h" #include "../readability/MagicNumbersCheck.h" #include "AvoidGotoCheck.h" @@ -47,6 +48,8 @@ CheckFactories.registerCheck( "cppcoreguidelines-narrowing-conversions"); CheckFactories.registerCheck("cppcoreguidelines-no-malloc"); + CheckFactories.registerCheck( + "cppcoreguidelines-non-private-member-variables-in-classes"); CheckFactories.registerCheck( "cppcoreguidelines-owning-memory"); CheckFactories.registerCheck( @@ -75,6 +78,16 @@ CheckFactories.registerCheck( "cppcoreguidelines-c-copy-assignment-signature"); } + + ClangTidyOptions getModuleOptions() override { + ClangTidyOptions Options; + auto &Opts = Options.CheckOptions; + + Opts["cppcoreguidelines-non-private-member-variables-in-classes." + "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "1"; + + return Options; + } }; // Register the LLVMTidyModule using this statically initialized variable. Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -6,6 +6,7 @@ MisplacedConstCheck.cpp NewDeleteOverloadsCheck.cpp NonCopyableObjects.cpp + NonPrivateMemberVariablesInClassesCheck.cpp RedundantExpressionCheck.cpp StaticAssertCheck.cpp ThrowByValueCatchByReferenceCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -14,6 +14,7 @@ #include "MisplacedConstCheck.h" #include "NewDeleteOverloadsCheck.h" #include "NonCopyableObjects.h" +#include "NonPrivateMemberVariablesInClassesCheck.h" #include "RedundantExpressionCheck.h" #include "StaticAssertCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" @@ -37,6 +38,8 @@ "misc-new-delete-overloads"); CheckFactories.registerCheck( "misc-non-copyable-objects"); + CheckFactories.registerCheck( + "misc-non-private-member-variables-in-classes"); CheckFactories.registerCheck( "misc-redundant-expression"); CheckFactories.registerCheck("misc-static-assert"); Index: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h @@ -0,0 +1,46 @@ +//===--- NonPrivateMemberVariablesInClassesCheck.h - clang-tidy -*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// This checker finds classes that not only contain the data +/// (non-static member variables), but also have logic (non-static member +/// functions), and diagnoses all member variables that have any other scope +/// other than `private`. They should be made `private`, and manipulated +/// exclusively via the member functions. +/// +/// Optionally, classes with all member variables being `public` could be +/// ignored and optionally all `public` member variables could be ignored. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-non-private-member-variables-in-classes.html +class NonPrivateMemberVariablesInClassesCheck : public ClangTidyCheck { +public: + NonPrivateMemberVariablesInClassesCheck(StringRef Name, + ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool IgnoreClassesWithAllMemberVariablesBeingPublic; + const bool IgnorePublicMemberVariables; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H Index: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp @@ -0,0 +1,93 @@ +//===--- NonPrivateMemberVariablesInClassesCheck.cpp - clang-tidy ---------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "NonPrivateMemberVariablesInClassesCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +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())) + .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) { + return Boolean; +} + +} // namespace + +NonPrivateMemberVariablesInClassesCheck:: + NonPrivateMemberVariablesInClassesCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IgnoreClassesWithAllMemberVariablesBeingPublic( + Options.get("IgnoreClassesWithAllMemberVariablesBeingPublic", false)), + IgnorePublicMemberVariables( + Options.get("IgnorePublicMemberVariables", false)) {} + +void NonPrivateMemberVariablesInClassesCheck::registerMatchers( + MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + // We can ignore structs/classes with all member variables being public. + auto ShouldIgnoreRecord = + allOf(boolean(IgnoreClassesWithAllMemberVariablesBeingPublic), + unless(hasNonPublicMemberVariable())); + + // 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. + auto RecordIsInteresting = + allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(), + unless(ShouldIgnoreRecord)); + + // 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())); + + Finder->addMatcher(cxxRecordDecl(RecordIsInteresting, + forEach(InterestingField.bind("field"))) + .bind("record"), + this); +} + +void NonPrivateMemberVariablesInClassesCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Field = Result.Nodes.getNodeAs("field"); + assert(Field && "We should have the field we are going to complain about"); + + diag(Field->getLocation(), "member variable %0 has %1 visibility") + << Field << Field->getAccess(); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -103,6 +103,13 @@ Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests ``absl::StrAppend()`` should be used instead. +- New :doc:`misc-non-private-member-variables-in-classes + ` check. + + Finds classes that not only contain the data (non-static member variables), + but also have logic (non-static member functions), and diagnoses all member + variables that have any other scope other than ``private``. + - New :doc:`modernize-concat-nested-namespaces ` check. @@ -122,6 +129,12 @@ Detects usage of magic numbers, numbers that are used as literals instead of introduced via constants or symbols. +- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes + ` + to :doc:`misc-non-private-member-variables-in-classes + ` + added. + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/cppcoreguidelines-non-private-member-variables-in-classes.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/cppcoreguidelines-non-private-member-variables-in-classes.rst @@ -0,0 +1,11 @@ +.. title:: clang-tidy - cppcoreguidelines-non-private-member-variables-in-classes +.. meta:: + :http-equiv=refresh: 5;URL=misc-non-private-member-variables-in-classes.html + +cppcoreguidelines-non-private-member-variables-in-classes +========================================================= + +The cppcoreguidelines-non-private-member-variables-in-classes check is an alias, +please see +`misc-non-private-member-variables-in-classes `_ +for more information. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -90,6 +90,7 @@ cppcoreguidelines-interfaces-global-init cppcoreguidelines-narrowing-conversions cppcoreguidelines-no-malloc + cppcoreguidelines-non-private-member-variables-in-classes (redirects to misc-non-private-member-variables-in-classes) cppcoreguidelines-owning-memory cppcoreguidelines-pro-bounds-array-to-pointer-decay cppcoreguidelines-pro-bounds-constant-array-index @@ -163,6 +164,7 @@ misc-misplaced-const misc-new-delete-overloads misc-non-copyable-objects + misc-non-private-member-variables-in-classes misc-redundant-expression misc-static-assert misc-throw-by-value-catch-by-reference Index: docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst @@ -0,0 +1,26 @@ +.. 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. + +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. Index: test/clang-tidy/misc-non-private-member-variables-in-classes.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-non-private-member-variables-in-classes.cpp @@ -0,0 +1,380 @@ +// RUN: %check_clang_tidy -check-suffixes=PUBLIC,NONPRIVATE,PROTECTED %s misc-non-private-member-variables-in-classes %t +// RUN: %check_clang_tidy -check-suffixes=PUBLIC,NONPRIVATE,PROTECTED %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}]}' -- +// RUN: %check_clang_tidy -check-suffixes=PUBLIC,PROTECTED %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: 1}]}' -- +// RUN: %check_clang_tidy -check-suffixes=PUBLIC,PROTECTED %s cppcoreguidelines-non-private-member-variables-in-classes %t -- -- +// RUN: %check_clang_tidy -check-suffixes=PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 1}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 0}]}' -- +// RUN: %check_clang_tidy -check-suffixes=PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 1}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 1}]}' -- + +//----------------------------------------------------------------------------// + +// Only data, do not warn + +struct S0 { + int S0_v0; + +public: + int S0_v1; + +protected: + int S0_v2; + +private: + int S0_v3; +}; + +class S1 { + int S1_v0; + +public: + int S1_v1; + +protected: + int S1_v2; + +private: + int S1_v3; +}; + +//----------------------------------------------------------------------------// + +// All functions are static, do not warn. + +struct S2 { + static void S2_m0(); + int S2_v0; + +public: + static void S2_m1(); + int S2_v1; + +protected: + static void S2_m3(); + int S2_v2; + +private: + static void S2_m4(); + int S2_v3; +}; + +class S3 { + static void S3_m0(); + int S3_v0; + +public: + static void S3_m1(); + int S3_v1; + +protected: + static void S3_m3(); + int S3_v2; + +private: + static void S3_m4(); + int S3_v3; +}; + +//============================================================================// + +// union != struct/class. do not diagnose. + +union U0 { + void U0_m0(); + int U0_v0; + +public: + void U0_m1(); + int U0_v1; + +protected: + void U0_m2(); + int U0_v2; + +private: + void U0_m3(); + int U0_v3; +}; + +//============================================================================// + +// Has non-static method with default visibility. + +struct S4 { + void S4_m0(); + + int S4_v0; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S4_v0' has public visibility +public: + int S4_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S4_v1' has public visibility +protected: + int S4_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S4_v2' has protected visibility +private: + int S4_v3; +}; + +class S5 { + void S5_m0(); + + int S5_v0; + +public: + int S5_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S5_v1' has public visibility +protected: + int S5_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S5_v2' has protected visibility +private: + int S5_v3; +}; + +//----------------------------------------------------------------------------// + +// Has non-static method with public visibility. + +struct S6 { + int S6_v0; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S6_v0' has public visibility +public: + void S6_m0(); + int S6_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S6_v1' has public visibility +protected: + int S6_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S6_v2' has protected visibility +private: + int S6_v3; +}; + +class S7 { + int S7_v0; + +public: + void S7_m0(); + int S7_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S7_v1' has public visibility +protected: + int S7_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S7_v2' has protected visibility +private: + int S7_v3; +}; + +//----------------------------------------------------------------------------// + +// Has non-static method with protected visibility. + +struct S8 { + int S8_v0; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S8_v0' has public visibility +public: + int S8_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S8_v1' has public visibility +protected: + void S8_m0(); + int S8_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S8_v2' has protected visibility +private: + int S8_v3; +}; + +class S9 { + int S9_v0; + +public: + int S9_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S9_v1' has public visibility +protected: + void S9_m0(); + int S9_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S9_v2' has protected visibility +private: + int S9_v3; +}; + +//----------------------------------------------------------------------------// + +// Has non-static method with private visibility. + +struct S10 { + int S10_v0; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S10_v0' has public visibility +public: + int S10_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S10_v1' has public visibility +protected: + int S10_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S10_v2' has protected visibility +private: + void S10_m0(); + int S10_v3; +}; + +class S11 { + int S11_v0; + +public: + int S11_v1; + // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S11_v1' has public visibility +protected: + int S11_v2; + // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S11_v2' has protected visibility +private: + void S11_m0(); + int S11_v3; +}; + +//============================================================================// + +// Static variables are ignored. +// Has non-static methods and static variables. + +struct S12 { + void S12_m0(); + static int S12_v0; + +public: + void S12_m1(); + static int S12_v1; + +protected: + void S12_m2(); + static int S12_v2; + +private: + void S12_m3(); + static int S12_v3; +}; + +class S13 { + void S13_m0(); + static int S13_v0; + +public: + void S13_m1(); + static int S13_v1; + +protected: + void S13_m2(); + static int S13_v2; + +private: + void S13_m3(); + static int S13_v3; +}; + +struct S14 { + void S14_m0(); + int S14_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S14_v0' has public visibility + +public: + void S14_m1(); + int S14_v1; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S14_v1' has public visibility + +protected: + void S14_m2(); + +private: + void S14_m3(); +}; + +class S15 { + void S15_m0(); + +public: + void S15_m1(); + int S15_v1; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S15_v1' has public visibility + +protected: + void S15_m2(); + +private: + void S15_m3(); +}; + +//----------------------------------------------------------------------------// + +template +struct S97 { + void method(); + T S97_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:5: warning: member variable 'S97_v0' has public visibility +}; + +template struct S97; + +template <> +struct S97 { + void method(); + double S97d_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:10: warning: member variable 'S97d_v0' has public visibility +}; + +//----------------------------------------------------------------------------// + +#define FIELD(x) int x; + +// Do diagnose fields originating from macros. +struct S98 { + void method(); + FIELD(S98_v0); + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:9: warning: member variable 'S98_v0' has public visibility +}; + +//----------------------------------------------------------------------------// + +// Don't look in descendant classes. +class S99 { + void method(); + + struct S99_0 { + int S99_S0_v0; + }; + +public: + struct S99_1 { + int S99_S0_v0; + }; + +protected: + struct S99_2 { + int S99_S0_v0; + }; + +private: + struct S99_3 { + int S99_S0_v0; + }; +}; + +//----------------------------------------------------------------------------// + +// Only diagnose once, don't let the inheritance fool you. +struct S100 { + int S100_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S100_v0' has public visibility + void m0(); +}; +struct S101_default_inheritance : S100 { + int S101_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S101_v0' has public visibility + void m1(); +}; +struct S102_public_inheritance : public S100 { + int S102_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S102_v0' has public visibility + void m1(); +}; +struct S103_protected_inheritance : protected S100 { + int S103_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S103_v0' has public visibility + void m1(); +}; +struct S104_private_inheritance : private S100 { + int S104_v0; + // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S104_v0' has public visibility + void m1(); +};