courbet created this revision.
courbet added reviewers: flx, aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
courbet requested review of this revision.
Herald added a project: clang-tools-extra.
This includes modifying `DeclRefExprUtils` to handle more cases.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114539
Files:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -0,0 +1,315 @@
+#include "../clang-tidy/utils/DeclRefExprUtils.h"
+#include "ClangTidyDiagnosticConsumer.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+
+class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
+public:
+ ConstReferenceDeclRefExprsTransform(StringRef CheckName,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(CheckName, Context) {}
+
+ void registerMatchers(MatchFinder *Finder) override {
+ Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+ }
+
+ void check(const MatchFinder::MatchResult &Result) override {
+ const auto *D = Result.Nodes.getNodeAs<VarDecl>("var");
+ using utils::decl_ref_expr::constReferenceDeclRefExprs;
+ const auto const_decrefexprs = constReferenceDeclRefExprs(
+ *D, *cast<FunctionDecl>(D->getDeclContext())->getBody(),
+ *Result.Context);
+
+ for (const DeclRefExpr *const Expr : const_decrefexprs) {
+ assert(Expr);
+ diag(Expr->getBeginLoc(), "const usage")
+ << FixItHint::CreateInsertion(Expr->getBeginLoc(), "/*const*/");
+ }
+ }
+};
+} // namespace
+
+namespace test {
+
+void RunTest(StringRef Snippet) {
+
+ StringRef CommonCode = R"(
+ struct ConstTag{};
+ struct NonConstTag{};
+
+ struct S {
+ void constMethod() const;
+ void nonConstMethod();
+
+ void operator()(ConstTag) const;
+ void operator()(NonConstTag);
+
+ void operator[](int);
+ void operator[](int) const;
+
+ bool operator==(const S&) const;
+
+ int int_member;
+ int* ptr_member;
+
+ };
+
+ struct Derived : public S {
+
+ };
+
+ void useVal(S);
+ void useRef(S&);
+ void usePtr(S*);
+ void usePtrPtr(S**);
+ void usePtrConstPtr(S* const*);
+ void useConstRef(const S&);
+ void useConstPtr(const S*);
+ void useConstPtrRef(const S*&);
+ void useConstPtrPtr(const S**);
+ void useConstPtrConstRef(const S* const&);
+ void useConstPtrConstPtr(const S* const*);
+
+ void useInt(int);
+ void useIntRef(int&);
+ void useIntConstRef(const int&);
+ void useIntPtr(int*);
+ void useIntConstPtr(const int*);
+
+ )";
+
+ std::string Code = (CommonCode + Snippet).str();
+
+ llvm::SmallVector<StringRef, 1> Parts;
+ StringRef(Code).split(Parts, "/*const*/");
+
+ EXPECT_EQ(Code, runCheckOnCode<ConstReferenceDeclRefExprsTransform>(
+ join(Parts, "")));
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ConstValueVar) {
+ RunTest(R"(
+ void f(const S target) {
+ useVal(/*const*/target);
+ useConstRef(/*const*/target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
+ /*const*/target.constMethod();
+ /*const*/target(ConstTag{});
+ /*const*/target[42];
+ useConstRef((/*const*/target));
+ (/*const*/target).constMethod();
+ (void)(/*const*/target == /*const*/target);
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ S copy1 = /*const*/target;
+ S copy2(/*const*/target);
+ useInt(/*const*/target.int_member);
+ useIntConstRef(/*const*/target.int_member);
+ useIntPtr(target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ConstRefVar) {
+ RunTest(R"(
+ void f(const S& target) {
+ useVal(/*const*/target);
+ useConstRef(/*const*/target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
+ /*const*/target.constMethod();
+ /*const*/target(ConstTag{});
+ /*const*/target[42];
+ useConstRef((/*const*/target));
+ (/*const*/target).constMethod();
+ (void)(/*const*/target == /*const*/target);
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ S copy1 = /*const*/target;
+ S copy2(/*const*/target);
+ useInt(/*const*/target.int_member);
+ useIntConstRef(/*const*/target.int_member);
+ useIntPtr(target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
+ RunTest(R"(
+ void f(S target, const S& other) {
+ useConstRef(/*const*/target);
+ useVal(/*const*/target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
+ /*const*/target.constMethod();
+ target.nonConstMethod();
+ /*const*/target(ConstTag{});
+ target[42];
+ /*const*/target(ConstTag{});
+ target(NonConstTag{});
+ useRef(target);
+ usePtr(&target);
+ useConstRef((/*const*/target));
+ (/*const*/target).constMethod();
+ (void)(/*const*/target == /*const*/target);
+ (void)(/*const*/target == other);
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ S copy1 = /*const*/target;
+ S copy2(/*const*/target);
+ useInt(/*const*/target.int_member);
+ useIntConstRef(/*const*/target.int_member);
+ useIntPtr(target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, RefVar) {
+ RunTest(R"(
+ void f(S& target) {
+ useVal(/*const*/target);
+ useConstRef(/*const*/target);
+ useConstPtr(&/*const*/target);
+ useConstPtrConstRef(&/*const*/target);
+ /*const*/target.constMethod();
+ target.nonConstMethod();
+ /*const*/target(ConstTag{});
+ target[42];
+ useConstRef((/*const*/target));
+ (/*const*/target).constMethod();
+ (void)(/*const*/target == /*const*/target);
+ (void)/*const*/target;
+ (void)&/*const*/target;
+ (void)*&/*const*/target;
+ S copy1 = /*const*/target;
+ S copy2(/*const*/target);
+ useInt(/*const*/target.int_member);
+ useIntConstRef(/*const*/target.int_member);
+ useIntPtr(target.ptr_member);
+ useIntConstPtr(&/*const*/target.int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
+ RunTest(R"(
+ void f(S* target) {
+ useVal(*/*const*/target);
+ useConstRef(*/*const*/target);
+ useConstPtr(/*const*/target);
+ useConstPtrConstRef(/*const*/target);
+ /*const*/target->constMethod();
+ target->nonConstMethod();
+ (*/*const*/target)(ConstTag{});
+ (*target)[42];
+ target->operator[](42);
+ useConstRef((*/*const*/target));
+ (/*const*/target)->constMethod();
+ (void)(*/*const*/target == */*const*/target);
+ (void)*/*const*/target;
+ (void)/*const*/target;
+ S copy1 = */*const*/target;
+ S copy2(*/*const*/target);
+ useInt(/*const*/target->int_member);
+ useIntConstRef(/*const*/target->int_member);
+ useIntPtr(target->ptr_member);
+ useIntConstPtr(&/*const*/target->int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ConstPtrVar) {
+ RunTest(R"(
+ void f(const S* target) {
+ useVal(*/*const*/target);
+ useConstRef(*/*const*/target);
+ useConstPtr(/*const*/target);
+ useConstPtrRef(target);
+ useConstPtrPtr(&target);
+ useConstPtrConstPtr(&/*const*/target);
+ useConstPtrConstRef(/*const*/target);
+ /*const*/target->constMethod();
+ (*/*const*/target)(ConstTag{});
+ (*/*const*/target)[42];
+ /*const*/target->operator[](42);
+ (void)(*/*const*/target == */*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ if(/*const*/target) {}
+ S copy1 = */*const*/target;
+ S copy2(*/*const*/target);
+ useInt(/*const*/target->int_member);
+ useIntConstRef(/*const*/target->int_member);
+ useIntPtr(target->ptr_member);
+ useIntConstPtr(&/*const*/target->int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ConstPtrPtrVar) {
+ RunTest(R"(
+ void f(const S** target) {
+ useVal(**/*const*/target);
+ useConstRef(**/*const*/target);
+ useConstPtr(*/*const*/target);
+ useConstPtrRef(*target);
+ useConstPtrPtr(target);
+ useConstPtrConstPtr(/*const*/target);
+ useConstPtrConstRef(*/*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ (void)**/*const*/target;
+ if(/*const*/target) {}
+ if(*/*const*/target) {}
+ S copy1 = **/*const*/target;
+ S copy2(**/*const*/target);
+ useInt((*/*const*/target)->int_member);
+ useIntConstRef((*/*const*/target)->int_member);
+ useIntPtr((*target)->ptr_member);
+ useIntConstPtr(&(*/*const*/target)->int_member);
+ }
+)");
+}
+
+TEST(ConstReferenceDeclRefExprsTest, ConstPtrConstPtrVar) {
+ RunTest(R"(
+ void f(const S* const* target) {
+ useVal(**/*const*/target);
+ useConstRef(**/*const*/target);
+ useConstPtr(*/*const*/target);
+ useConstPtrConstPtr(/*const*/target);
+ useConstPtrConstRef(*/*const*/target);
+ (void)/*const*/target;
+ (void)*/*const*/target;
+ (void)**/*const*/target;
+ if(/*const*/target) {}
+ if(*/*const*/target) {}
+ S copy1 = **/*const*/target;
+ S copy2(**/*const*/target);
+ useInt((*/*const*/target)->int_member);
+ useIntConstRef((*/*const*/target)->int_member);
+ useIntPtr((*target)->ptr_member);
+ useIntConstPtr(&(*/*const*/target)->int_member);
+ }
+)");
+}
+
+} // namespace test
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===================================================================
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -20,6 +20,7 @@
AddConstTest.cpp
ClangTidyDiagnosticConsumerTest.cpp
ClangTidyOptionsTest.cpp
+ DeclRefExprUtilsTest.cpp
IncludeInserterTest.cpp
GlobListTest.cpp
GoogleModuleTest.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -227,23 +227,23 @@
void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
const auto AutoAssigned = (*C)[42];
- // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
- // TODO-FIXES: const auto& AutoAssigned = (*C)[42];
+ // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoAssigned'
+ // CHECK-FIXES: const auto& AutoAssigned = (*C)[42];
AutoAssigned.constMethod();
const auto AutoCopyConstructed((*C)[42]);
- // TODO-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
- // TODO-FIXES: const auto& AutoCopyConstructed((*C)[42]);
+ // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'AutoCopyConstructed'
+ // CHECK-FIXES: const auto& AutoCopyConstructed((*C)[42]);
AutoCopyConstructed.constMethod();
const ExpensiveToCopyType VarAssigned = C->operator[](42);
- // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
- // TODO-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C->operator[](42);
VarAssigned.constMethod();
const ExpensiveToCopyType VarCopyConstructed(C->operator[](42));
- // TODO-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
- // TODO-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->operator[](42));
VarCopyConstructed.constMethod();
}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -37,6 +37,108 @@
Nodes.insert(Match.getNodeAs<Node>(ID));
}
+// A matcher that matches DeclRefExprs that are only used in a const way.
+// When the variable is a pointer, we require all nesting levels to be const.
+AST_MATCHER(DeclRefExpr, isOnlyUsedAsConst) {
+ // We walk up the parents of the DeclRefExpr. When we find a node that has a
+ // parent which is not cast or deref/addrof, we check whether that node can
+ // modify the variable. If that's the case, then the variable can be modified.
+ // Else, we continue scanning the parents.
+ std::vector<const Expr *> Stack = {&Node};
+ auto &Ctx = Finder->getASTContext();
+
+ // Returns true if it this is a cast or deref/addrof.
+ const auto IsVetted = [](const Expr *const E) {
+ if (const auto *const Paren = dyn_cast<ParenExpr>(E)) {
+ return true; // No-op.
+ }
+ if (const auto *const Cast = dyn_cast<CastExpr>(E)) {
+ // Bail out on casts that we cannot check. Defaults to safe.
+ switch (Cast->getCastKind()) {
+ case CK_LValueToRValue:
+ case CK_NoOp:
+ case CK_BaseToDerived:
+ case CK_DerivedToBase:
+ case CK_UncheckedDerivedToBase:
+ case CK_Dynamic:
+ case CK_BaseToDerivedMemberPointer:
+ case CK_DerivedToBaseMemberPointer:
+ case CK_ToVoid:
+ case CK_PointerToBoolean:
+ return true;
+ default:
+ return false;
+ }
+ }
+ if (const auto *const Op = dyn_cast<UnaryOperator>(E)) {
+ switch (Op->getOpcode()) {
+ case UO_AddrOf:
+ case UO_Deref:
+ return true;
+ default:
+ return false;
+ }
+ }
+ if (const auto *const Member = dyn_cast<MemberExpr>(E)) {
+ if (const auto *const Method =
+ dyn_cast<CXXMethodDecl>(Member->getMemberDecl())) {
+ return Method->isConst();
+ }
+ return true;
+ }
+ return false;
+ };
+
+ while (!Stack.empty()) {
+ const Expr *const E = Stack.back();
+ Stack.pop_back();
+ const auto Parents = Ctx.getParents(*E);
+ bool HasUnvettedParents = false;
+ for (const auto &Parent : Parents) {
+ const Expr *const ParentExpr = Parent.get<Expr>();
+ if (ParentExpr == nullptr) {
+ continue; // ParentExpr's value is unused.
+ }
+ if (IsVetted(ParentExpr)) {
+ Stack.push_back(ParentExpr);
+ } else {
+ HasUnvettedParents = true;
+ }
+ }
+ if (HasUnvettedParents) {
+ // See if the expression is modifiable.
+ SourceLocation Loc;
+ switch (
+ E->ClassifyModifiable(Finder->getASTContext(), Loc).getModifiable()) {
+ case Expr::Classification::CM_Untested:
+ case Expr::Classification::CM_Modifiable:
+ return false;
+ case Expr::Classification::CM_ConstQualified:
+ case Expr::Classification::CM_ConstQualifiedField:
+ case Expr::Classification::CM_ConstAddrSpace:
+ case Expr::Classification::CM_Function:
+ case Expr::Classification::CM_RValue:
+ case Expr::Classification::CM_LValueCast:
+ break;
+ // FIXME: Not sure about these.
+ case Expr::Classification::CM_NoSetterProperty:
+ case Expr::Classification::CM_ArrayType:
+ case Expr::Classification::CM_IncompleteType:
+ return false;
+ }
+ // Check that the type prevents modification through pointers.
+ for (QualType Ty = E->getType().getCanonicalType()->getPointeeType();
+ !Ty.isNull(); Ty = Ty->getPointeeType()) {
+ Ty = Ty.getCanonicalType();
+ if (!Ty.isConstQualified()) {
+ return false;
+ }
+ };
+ }
+ }
+ return true;
+}
+
} // namespace
// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
@@ -44,44 +146,12 @@
SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
ASTContext &Context) {
- auto DeclRefToVar =
- declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
- auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
- // Match method call expressions where the variable is referenced as the this
- // implicit object argument and operator call expression for member operators
- // where the variable is the 0-th argument.
- auto Matches = match(
- findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
- cxxOperatorCallExpr(ConstMethodCallee,
- hasArgument(0, DeclRefToVar))))),
- Stmt, Context);
+ auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl))),
+ isOnlyUsedAsConst())
+ .bind("declRef")),
+ Stmt, Context);
SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
extractNodesByIdTo(Matches, "declRef", DeclRefs);
- auto ConstReferenceOrValue =
- qualType(anyOf(matchers::isReferenceToConst(),
- unless(anyOf(referenceType(), pointerType(),
- substTemplateTypeParmType()))));
- auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
- ConstReferenceOrValue,
- substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
- auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
- DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
- Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- // References and pointers to const assignments.
- Matches =
- match(findAll(declStmt(
- has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
- hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- Matches =
- match(findAll(declStmt(has(varDecl(
- hasType(qualType(matchers::isPointerToConst())),
- hasInitializer(ignoringImpCasts(unaryOperator(
- hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
return DeclRefs;
}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,7 +86,8 @@
const auto MethodDecl =
cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst())))
.bind(MethodDeclId);
- const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+ const auto ReceiverExpr =
+ ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId))));
const auto ReceiverType =
hasCanonicalType(recordType(hasDeclaration(namedDecl(
unless(matchers::matchesAnyListedName(ExcludedContainerTypes))))));
@@ -94,8 +95,15 @@
return expr(anyOf(
cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
thisPointerType(ReceiverType)),
- cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
- hasArgument(0, hasType(ReceiverType)))));
+ cxxOperatorCallExpr(
+ callee(MethodDecl),
+ // We allow operators to be called on objects or
+ // dereference of pointer-to-object.
+ hasArgument(0, expr(hasType(ReceiverType),
+ anyOf(ReceiverExpr,
+ ignoringParenImpCasts(unaryOperator(
+ hasOperatorName("*"),
+ hasUnaryOperand(ReceiverExpr)))))))));
}
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
