diff --git a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h @@ -37,7 +37,7 @@ public: ReservedIdentifierCheck(StringRef Name, ClangTidyContext *Context); - void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void storeCheckOptions(ClangTidyOptions::OptionMap &Opts) override; private: llvm::Optional diff --git a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp @@ -44,7 +44,8 @@ AllowedIdentifiers(utils::options::parseStringList( Options.get("AllowedIdentifiers", ""))) {} -void ReservedIdentifierCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { +void ReservedIdentifierCheck::storeCheckOptions( + ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "Invert", Invert); Options.store(Opts, "AllowedIdentifiers", utils::options::serializeStringList(AllowedIdentifiers)); diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h @@ -35,7 +35,7 @@ IdentifierNamingCheck(StringRef Name, ClangTidyContext *Context); ~IdentifierNamingCheck(); - void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void storeCheckOptions(ClangTidyOptions::OptionMap &Opts) override; enum CaseType { CT_AnyCase = 0, diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -155,7 +155,8 @@ IdentifierNamingCheck::~IdentifierNamingCheck() = default; -void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { +void IdentifierNamingCheck::storeCheckOptions( + ClangTidyOptions::OptionMap &Opts) { auto const toString = [](CaseType Type) { switch (Type) { case CT_AnyCase: diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h --- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h @@ -33,6 +33,7 @@ /// class will do the matching and call the derived class' /// GetDeclFailureInfo() and GetMacroFailureInfo() for determining whether a /// given identifier passes or fails the check. + void storeOptions(ClangTidyOptions::OptionMap &Opts) override final; void registerMatchers(ast_matchers::MatchFinder *Finder) override final; void check(const ast_matchers::MatchFinder::MatchResult &Result) override final; @@ -40,6 +41,8 @@ Preprocessor *ModuleExpanderPP) override final; void onEndOfTranslationUnit() override final; + virtual void storeCheckOptions(ClangTidyOptions::OptionMap &Opts) {} + /// This enum will be used in %select of the diagnostic message. /// Each value below IgnoreFailureThreshold should have an error message. enum class ShouldFixStatus { @@ -142,6 +145,7 @@ private: NamingCheckFailureMap NamingCheckFailures; + const bool AggressiveDependentMemberLookup; }; } // namespace tidy diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp --- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp @@ -14,8 +14,7 @@ #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/DenseMapInfo.h" -#include "llvm/Support/Debug.h" -#include "llvm/Support/Format.h" +#include "llvm/Support/Error.h" #define DEBUG_TYPE "clang-tidy" @@ -93,9 +92,17 @@ RenamerClangTidyCheck::RenamerClangTidyCheck(StringRef CheckName, ClangTidyContext *Context) - : ClangTidyCheck(CheckName, Context) {} + : ClangTidyCheck(CheckName, Context), + AggressiveDependentMemberLookup( + Options.get("AggressiveDependentMemberLookup", false)) {} RenamerClangTidyCheck::~RenamerClangTidyCheck() = default; +void RenamerClangTidyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AggressiveDependentMemberLookup", + AggressiveDependentMemberLookup); + storeCheckOptions(Opts); +} + void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher(namedDecl().bind("decl"), this); Finder->addMatcher(usingDecl().bind("using"), this); @@ -106,20 +113,13 @@ this); Finder->addMatcher(typeLoc().bind("typeLoc"), this); Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this); + auto MemberRestrictions = + expr(unless(anyOf(hasAncestor(decl(isImplicit())), + hasAncestor(functionDecl(isDefaulted()))))); + Finder->addMatcher(memberExpr(MemberRestrictions).bind("memberExpr"), this); Finder->addMatcher( - functionDecl(unless(cxxMethodDecl(isImplicit())), - hasBody(forEachDescendant(memberExpr().bind("memberExpr")))), - this); - Finder->addMatcher( - cxxConstructorDecl( - unless(isImplicit()), - forEachConstructorInitializer( - allOf(isWritten(), withInitializer(forEachDescendant( - memberExpr().bind("memberExpr")))))), + cxxDependentScopeMemberExpr(MemberRestrictions).bind("depMemberExpr"), this); - Finder->addMatcher(fieldDecl(hasInClassInitializer( - forEachDescendant(memberExpr().bind("memberExpr")))), - this); } void RenamerClangTidyCheck::registerPPCallbacks( @@ -169,6 +169,49 @@ Range, SourceMgr); } +const NamedDecl *findDecl(const RecordDecl &RecDecl, StringRef DeclName) { + for (const Decl *D : RecDecl.decls()) { + if (const auto *ND = dyn_cast(D)) { + if (ND->getDeclName().isIdentifier() && ND->getName().equals(DeclName)) + return ND; + } + } + return nullptr; +} + +llvm::ErrorOr +findDeclInBases(const CXXRecordDecl &Parent, StringRef DeclName, + bool AggressiveTemplateLookup) { + if (const NamedDecl *InClassRef = findDecl(Parent, DeclName)) + return InClassRef; + const NamedDecl *Found = nullptr; + + for (CXXBaseSpecifier Base : Parent.bases()) { + const auto *Record = Base.getType()->getAsCXXRecordDecl(); + if (!Record && AggressiveTemplateLookup) { + if (const auto *TST = + Base.getType()->getAs()) { + if (const auto *TD = llvm::dyn_cast_or_null( + TST->getTemplateName().getAsTemplateDecl())) + Record = TD->getTemplatedDecl(); + } + } + if (!Record) + continue; + if (auto Search = + findDeclInBases(*Record, DeclName, AggressiveTemplateLookup)) { + if (*Search) { + if (Found) + return std::error_code(); + Found = *Search; + continue; + } + } else + return std::error_code(); + } + return Found; // If nullptr decl wasnt found; +} + void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Decl = Result.Nodes.getNodeAs("classRef")) { @@ -179,7 +222,7 @@ for (const auto *Init : Decl->inits()) { if (!Init->isWritten() || Init->isInClassMemberInitializer()) continue; - if (const FieldDecl *FD = Init->getAnyMember()) + if (const FieldDecl *FD = Init->getMember()) addUsage(NamingCheckFailures, FD, SourceRange(Init->getMemberLocation())); // Note: delegating constructors and base class initializers are handled @@ -271,6 +314,32 @@ return; } + if (const auto *DepMemberRef = + Result.Nodes.getNodeAs( + "depMemberExpr")) { + QualType BaseType = DepMemberRef->isArrow() + ? DepMemberRef->getBaseType()->getPointeeType() + : DepMemberRef->getBaseType(); + if (BaseType.isNull()) + return; + const CXXRecordDecl *Base = BaseType.getTypePtr()->getAsCXXRecordDecl(); + if (!Base) + return; + DeclarationName DeclName = DepMemberRef->getMemberNameInfo().getName(); + if (!DeclName.isIdentifier()) + return; + StringRef DependentName = DeclName.getAsIdentifierInfo()->getName(); + + if (llvm::ErrorOr Resolved = findDeclInBases( + *Base, DependentName, AggressiveDependentMemberLookup)) { + if (*Resolved) + addUsage(NamingCheckFailures, *Resolved, + DepMemberRef->getMemberNameInfo().getSourceRange(), + Result.SourceManager); + } + return; + } + if (const auto *Decl = Result.Nodes.getNodeAs("decl")) { if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit()) return; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,12 @@ Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:'readability-identifier-naming + ` check. + + Now able to rename member references in class template definitions with + explicit access. + - Improved :doc:`readability-redundant-string-init ` check now supports a `StringNames` option enabling its application to custom string classes. The diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst --- a/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst @@ -36,6 +36,7 @@ The following options are describe below: - :option:`AbstractClassCase`, :option:`AbstractClassPrefix`, :option:`AbstractClassSuffix` + - :option:`AggressiveDependentMemberLookup` - :option:`ClassCase`, :option:`ClassPrefix`, :option:`ClassSuffix` - :option:`ClassConstantCase`, :option:`ClassConstantPrefix`, :option:`ClassConstantSuffix` - :option:`ClassMemberCase`, :option:`ClassMemberPrefix`, :option:`ClassMemberSuffix` @@ -127,6 +128,64 @@ pre_abstract_class_post(); }; +.. option:: AggressiveDependentMemberLookup + + When set to `1` the check will look in dependent base classes for dependent + member references that need changing. This can lead to errors with template + specializations so the default value is `0`. + +For example using values of: + + - ClassMemberCase of ``lower_case`` + +Before: + +.. code-block:: c++ + + template + struct Base { + T BadNamedMember; + }; + + template + struct Derived : Base { + void reset() { + this->BadNamedMember = 0; + } + }; + +After if AggressiveDependentMemberLookup is ``0``: + +.. code-block:: c++ + + template + struct Base { + T bad_named_member; + }; + + template + struct Derived : Base { + void reset() { + this->BadNamedMember = 0; + } + }; + +After if AggressiveDependentMemberLookup is ``1``: + +.. code-block:: c++ + + template + struct Base { + T bad_named_member; + }; + + template + struct Derived : Base { + void reset() { + this->bad_named_member = 0; + } + }; + .. option:: ClassCase When defined, the check will ensure class names conform to the diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp @@ -1,7 +1,9 @@ // RUN: %check_clang_tidy %s readability-identifier-naming %t -- \ // RUN: -config='{CheckOptions: [ \ // RUN: {key: readability-identifier-naming.MemberCase, value: CamelCase}, \ -// RUN: {key: readability-identifier-naming.ParameterCase, value: CamelCase} \ +// RUN: {key: readability-identifier-naming.ParameterCase, value: CamelCase}, \ +// RUN: {key: readability-identifier-naming.MethodCase, value: camelBack}, \ +// RUN: {key: readability-identifier-naming.AggressiveDependentMemberLookup, value: 1} \ // RUN: ]}' -- -fno-delayed-template-parsing int set_up(int); @@ -63,32 +65,23 @@ // CHECK-FIXES: {{^}} int getBar2() const { return this->BarBaz; } // comment-9 }; -TempTest x; //force an instantiation - -int blah() { - return x.getBar2(); // gotta have a reference to the getBar2 so that the - // compiler will generate the function and resolve - // the DependantScopeMemberExpr -} - namespace Bug41122 { namespace std { // for this example we aren't bothered about how std::vector is treated -template //NOLINT -class vector { //NOLINT -public: - void push_back(bool); //NOLINT - void pop_back(); //NOLINT -}; //NOLINT -}; // namespace std +template // NOLINT +struct vector { // NOLINT + void push_back(bool); // NOLINT + void pop_back(); // NOLINT +}; // NOLINT +}; // namespace std -class Foo { +class Foo { std::vector &stack; // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for member 'stack' [readability-identifier-naming] public: Foo(std::vector &stack) - // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: invalid case style for parameter 'stack' [readability-identifier-naming] + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: invalid case style for parameter 'stack' [readability-identifier-naming] // CHECK-FIXES: {{^}} Foo(std::vector &Stack) : stack(stack) { // CHECK-FIXES: {{^}} : Stack(Stack) { @@ -134,4 +127,92 @@ void foo() { Container container; } -}; // namespace CtorInits +} // namespace CtorInits + +namespace resolved_dependance { +template +struct A0 { + int value; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value' + A0 &operator=(const A0 &Other) { + value = Other.value; // A0 + this->value = Other.value; // A0 + // CHECK-FIXES: {{^}} Value = Other.Value; // A0 + // CHECK-FIXES-NEXT: {{^}} this->Value = Other.Value; // A0 + return *this; + } + void outOfLineReset(); +}; + +template +void A0::outOfLineReset() { + this->value -= value; // A0 + // CHECK-FIXES: {{^}} this->Value -= Value; // A0 +} + +template +struct A1 { + int value; // A1 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value' + // CHECK-FIXES: {{^}} int Value; // A1 + int GetValue() const { return value; } // A1 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for method 'GetValue' + // CHECK-FIXES {{^}} int getValue() const { return Value; } // A1 + void SetValue(int Value) { this->value = Value; } // A1 + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'SetValue' + // CHECK-FIXES {{^}} void setValue(int Value) { this->Value = Value; } // A1 + A1 &operator=(const A1 &Other) { + this->SetValue(Other.GetValue()); // A1 + this->value = Other.value; // A1 + // CHECK-FIXES: {{^}} this->setValue(Other.getValue()); // A1 + // CHECK-FIXES-NEXT: {{^}} this->Value = Other.Value; // A1 + return *this; + } + void outOfLineReset(); +}; + +template +void A1::outOfLineReset() { + this->value -= value; // A1 + // CHECK-FIXES: {{^}} this->Value -= Value; // A1 +} + +template +struct A2 { + int value; // A2 + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value' + // CHECK-FIXES: {{^}} int Value; // A2 + A2 &operator=(const A2 &Other) { + value = Other.value; // A2 + this->value = Other.value; // A2 + // CHECK-FIXES: {{^}} Value = Other.Value; // A2 + // CHECK-FIXES-NEXT: {{^}} this->Value = Other.Value; // A2 + return *this; + } +}; + +// create some instances to check it works when instantiated. +A1 AInt{}; +A1 BInt = (AInt.outOfLineReset(), AInt); +A1 AUnsigned{}; +A1 BUnsigned = AUnsigned; +} // namespace resolved_dependance + +namespace unresolved_dependance { +template +struct DependentBase { + int depValue; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'depValue' + // CHECK-FIXES: {{^}} int DepValue; +}; + +template +struct Derived : DependentBase { + Derived &operator=(const Derived &Other) { + this->depValue = Other.depValue; + // CHECK-FIXES: {{^}} this->DepValue = Other.DepValue; + return *this; + } +}; + +} // namespace unresolved_dependance