Author: Nathan James Date: 2020-05-09T16:21:49+01:00 New Revision: 82ddae061b4ba11895756004d559160cbd519fff
URL: https://github.com/llvm/llvm-project/commit/82ddae061b4ba11895756004d559160cbd519fff DIFF: https://github.com/llvm/llvm-project/commit/82ddae061b4ba11895756004d559160cbd519fff.diff LOG: [clang-tidy] RenamerClangTidy now renames dependent member expr when the member can be resolved Summary: Sometimes in templated code Member references are reported as `DependentScopeMemberExpr` because that's what the standard dictates, however in many trivial cases it is easy to resolve the reference to its actual Member. Take this code: ``` template<typename T> class A{ int value; A& operator=(const A& Other){ value = Other.value; this->value = Other.value; return *this; } }; ``` When ran with `clang-tidy file.cpp -checks=readability-identifier-naming --config="{CheckOptions: [{key: readability-identifier-naming.MemberPrefix, value: m_}]}" -fix` Current behaviour: ``` template<typename T> class A{ int m_value; A& operator=(const A& Other){ m_value = Other.value; this->value = Other.value; return *this; } }; ``` As `this->value` and `Other.value` are Dependent they are ignored when creating the fix-its, however this can easily be resolved. Proposed behaviour: ``` template<typename T> class A{ int m_value; A& operator=(const A& Other){ m_value = Other.m_value; this->m_value = Other.m_value; return *this; } }; ``` Reviewers: aaron.ballman, JonasToth, alexfh, hokein, gribozavr2 Reviewed By: aaron.ballman Subscribers: merge_guards_bot, xazax.hun, cfe-commits Tags: #clang, #clang-tools-extra Differential Revision: https://reviews.llvm.org/D73052 Added: Modified: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp index d562d8c3d213..9619524a0c03 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp @@ -47,6 +47,7 @@ ReservedIdentifierCheck::ReservedIdentifierCheck(StringRef Name, Options.get("AllowedIdentifiers", ""))) {} void ReservedIdentifierCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + RenamerClangTidyCheck::storeOptions(Opts); Options.store(Opts, "Invert", Invert); Options.store(Opts, "AllowedIdentifiers", utils::options::serializeStringList(AllowedIdentifiers)); diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp index 9146e7356c9c..7c7de74a0c9e 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -170,6 +170,7 @@ IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name, IdentifierNamingCheck::~IdentifierNamingCheck() = default; void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + RenamerClangTidyCheck::storeOptions(Opts); for (size_t i = 0; i < SK_Count; ++i) { if (NamingStyles[i]) { if (NamingStyles[i]->Case) { diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp index 3523cf5dcf16..d8bdbcc8c25e 100644 --- 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/ADT/PointerIntPair.h" #define DEBUG_TYPE "clang-tidy" @@ -93,9 +92,16 @@ class RenamerClangTidyCheckPPCallbacks : public PPCallbacks { RenamerClangTidyCheck::RenamerClangTidyCheck(StringRef CheckName, ClangTidyContext *Context) - : ClangTidyCheck(CheckName, Context) {} + : ClangTidyCheck(CheckName, Context), + AggressiveDependentMemberLookup( + Options.getLocalOrGlobal("AggressiveDependentMemberLookup", false)) {} RenamerClangTidyCheck::~RenamerClangTidyCheck() = default; +void RenamerClangTidyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AggressiveDependentMemberLookup", + AggressiveDependentMemberLookup); +} + void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher(namedDecl().bind("decl"), this); Finder->addMatcher(usingDecl().bind("using"), this); @@ -106,20 +112,12 @@ void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) { this); Finder->addMatcher(typeLoc().bind("typeLoc"), this); Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this); + auto MemberRestrictions = + unless(forFunction(anyOf(isDefaulted(), isImplicit()))); + 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 +167,78 @@ static void addUsage(RenamerClangTidyCheck::NamingCheckFailureMap &Failures, Range, SourceMgr); } +const NamedDecl *findDecl(const RecordDecl &RecDecl, StringRef DeclName) { + for (const Decl *D : RecDecl.decls()) { + if (const auto *ND = dyn_cast<NamedDecl>(D)) { + if (ND->getDeclName().isIdentifier() && ND->getName().equals(DeclName)) + return ND; + } + } + return nullptr; +} + +namespace { +class NameLookup { + llvm::PointerIntPair<const NamedDecl *, 1, bool> Data; + +public: + explicit NameLookup(const NamedDecl *ND) : Data(ND, false) {} + explicit NameLookup(llvm::NoneType) : Data(nullptr, true) {} + explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {} + NameLookup() : NameLookup(nullptr) {} + + bool hasMultipleResolutions() const { return Data.getInt(); } + bool hasDecl() const { + assert(!hasMultipleResolutions() && "Found multiple decls"); + return Data.getPointer() != nullptr; + } + const NamedDecl *getDecl() const { + assert(!hasMultipleResolutions() && "Found multiple decls"); + return Data.getPointer(); + } + operator bool() const { return !hasMultipleResolutions(); } + const NamedDecl *operator*() const { return getDecl(); } +}; +} // namespace + +/// Returns a decl matching the \p DeclName in \p Parent or one of its base +/// classes. If \p AggressiveTemplateLookup is `true` then it will check +/// template dependent base classes as well. +/// If a matching decl is found in multiple base classes then it will return a +/// flag indicating the multiple resolutions. +NameLookup findDeclInBases(const CXXRecordDecl &Parent, StringRef DeclName, + bool AggressiveTemplateLookup) { + if (const NamedDecl *InClassRef = findDecl(Parent, DeclName)) + return NameLookup(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<TemplateSpecializationType>()) { + if (const auto *TD = llvm::dyn_cast_or_null<ClassTemplateDecl>( + TST->getTemplateName().getAsTemplateDecl())) + Record = TD->getTemplatedDecl(); + } + } + if (!Record) + continue; + if (auto Search = + findDeclInBases(*Record, DeclName, AggressiveTemplateLookup)) { + if (*Search) { + if (Found) + return NameLookup( + llvm::None); // Multiple decls found in diff erent base classes. + Found = *Search; + continue; + } + } else + return NameLookup(llvm::None); // Propagate multiple resolution back up. + } + return NameLookup(Found); // If nullptr, decl wasnt found. +} + void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Decl = Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) { @@ -272,6 +342,32 @@ void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) { return; } + if (const auto *DepMemberRef = + Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>( + "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 (NameLookup Resolved = findDeclInBases( + *Base, DependentName, AggressiveDependentMemberLookup)) { + if (*Resolved) + addUsage(NamingCheckFailures, *Resolved, + DepMemberRef->getMemberNameInfo().getSourceRange(), + Result.SourceManager); + } + return; + } + if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) { // Fix using namespace declarations. if (const auto *UsingNS = dyn_cast<UsingDirectiveDecl>(Decl)) diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h index a9182b11b602..98c3dc5c3a8d 100644 --- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h @@ -40,6 +40,10 @@ class RenamerClangTidyCheck : public ClangTidyCheck { Preprocessor *ModuleExpanderPP) override final; void onEndOfTranslationUnit() override final; + /// Derived classes that override this function should call this method from + /// the overridden method. + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + /// 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 +146,7 @@ class RenamerClangTidyCheck : public ClangTidyCheck { private: NamingCheckFailureMap NamingCheckFailures; + const bool AggressiveDependentMemberLookup; }; } // namespace tidy diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 9e4fa62b7ab6..1827dfe91338 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -176,6 +176,12 @@ New check aliases Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:'readability-identifier-naming + <clang-tidy/checks/readability-identifier-naming>` check. + + Now able to rename member references in class template definitions with + explicit access. + - Improved :doc:`readability-qualified-auto <clang-tidy/checks/readability-qualified-auto>` check now supports a `AddConstToQualified` to enable adding ``const`` qualifiers to variables 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 index 0998bf28c278..3d0cbca69f6d 100644 --- 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 @@ Options 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 @@ After: 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 <typename T> + struct Base { + T BadNamedMember; + }; + + template <typename T> + struct Derived : Base<T> { + void reset() { + this->BadNamedMember = 0; + } + }; + +After if AggressiveDependentMemberLookup is ``0``: + +.. code-block:: c++ + + template <typename T> + struct Base { + T bad_named_member; + }; + + template <typename T> + struct Derived : Base<T> { + void reset() { + this->BadNamedMember = 0; + } + }; + +After if AggressiveDependentMemberLookup is ``1``: + +.. code-block:: c++ + + template <typename T> + struct Base { + T bad_named_member; + }; + + template <typename T> + struct Derived : Base<T> { + 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 index 177b3e2116a8..a7b637c70b55 100644 --- 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 @@ class TempTest : public Foo { // CHECK-FIXES: {{^}} int getBar2() const { return this->BarBaz; } // comment-9 }; -TempTest<int> 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 <typename T> //NOLINT -class vector { //NOLINT -public: - void push_back(bool); //NOLINT - void pop_back(); //NOLINT -}; //NOLINT -}; // namespace std +template <typename T> // NOLINT +struct vector { // NOLINT + void push_back(bool); // NOLINT + void pop_back(); // NOLINT +}; // NOLINT +}; // namespace std -class Foo { +class Foo { std::vector<bool> &stack; // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for member 'stack' [readability-identifier-naming] public: Foo(std::vector<bool> &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<bool> &Stack) : stack(stack) { // CHECK-FIXES: {{^}} : Stack(Stack) { @@ -134,4 +127,92 @@ class Container { void foo() { Container<int, 5> container; } -}; // namespace CtorInits +} // namespace CtorInits + +namespace resolved_dependance { +template <typename T> +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 <typename T> +void A0<T>::outOfLineReset() { + this->value -= value; // A0 + // CHECK-FIXES: {{^}} this->Value -= Value; // A0 +} + +template <typename T> +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 <typename T> +void A1<T>::outOfLineReset() { + this->value -= value; // A1 + // CHECK-FIXES: {{^}} this->Value -= Value; // A1 +} + +template <unsigned T> +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<int> AInt{}; +A1<int> BInt = (AInt.outOfLineReset(), AInt); +A1<unsigned> AUnsigned{}; +A1<unsigned> BUnsigned = AUnsigned; +} // namespace resolved_dependance + +namespace unresolved_dependance { +template <typename T> +struct DependentBase { + int depValue; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'depValue' + // CHECK-FIXES: {{^}} int DepValue; +}; + +template <typename T> +struct Derived : DependentBase<T> { + Derived &operator=(const Derived &Other) { + this->depValue = Other.depValue; + // CHECK-FIXES: {{^}} this->DepValue = Other.DepValue; + return *this; + } +}; + +} // namespace unresolved_dependance _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
