njames93 updated this revision to Diff 257151.
njames93 added a comment.
Clean up code and remove unrelated changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73052/new/
https://reviews.llvm.org/D73052
Files:
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
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
Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp
+++ 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<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 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ 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 <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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -166,6 +166,12 @@
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
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
+++ 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
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ 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,17 @@
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);
+ storeCheckOptions(Opts);
+}
+
void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(namedDecl().bind("decl"), this);
Finder->addMatcher(usingDecl().bind("using"), this);
@@ -106,20 +113,12 @@
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 +168,77 @@
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:
+ NameLookup(const NamedDecl *ND) : Data(ND, false) {}
+ NameLookup(llvm::NoneType) : Data(nullptr, true) {}
+ 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 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 llvm::None; // Multiple decls found in different base classes.
+ Found = *Search;
+ continue;
+ }
+ } else
+ return llvm::None; // Propagate multiple resolution back up.
+ }
+ return 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 @@
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))
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ 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,
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -169,7 +169,8 @@
IdentifierNamingCheck::~IdentifierNamingCheck() = default;
-void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+void IdentifierNamingCheck::storeCheckOptions(
+ ClangTidyOptions::OptionMap &Opts) {
for (size_t i = 0; i < SK_Count; ++i) {
if (NamingStyles[i]) {
if (NamingStyles[i]->Case) {
Index: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
+++ 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<FailureInfo>
Index: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
@@ -46,7 +46,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));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
