diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp --- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp +++ b/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) { diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp --- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp +++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp @@ -37,6 +37,108 @@ Nodes.insert(Match.getNodeAs(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 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(E)) { + return true; // No-op. + } + if (const auto *const Cast = dyn_cast(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(E)) { + switch (Op->getOpcode()) { + case UO_AddrOf: + case UO_Deref: + return true; + default: + return false; + } + } + if (const auto *const Member = dyn_cast(E)) { + if (const auto *const Method = + dyn_cast(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(); + 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 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 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; } diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp @@ -227,23 +227,23 @@ void PositiveOperatorCallConstValueParam(const Container* 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(); } diff --git a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt b/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt --- a/clang-tools-extra/unittests/clang-tidy/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp new file mode 100644 --- /dev/null +++ b/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("var"); + using utils::decl_ref_expr::constReferenceDeclRefExprs; + const auto const_decrefexprs = constReferenceDeclRefExprs( + *D, *cast(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 Parts; + StringRef(Code).split(Parts, "/*const*/"); + + EXPECT_EQ(Code, runCheckOnCode( + 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