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,148 @@ 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) { + // The idea of the algorithm is that even if the root DeclRefExpr (`Node`) is + // modifiable, it can be cast to a const higher up in the AST. For example: + // + // |-CallExpr 'void' + // | |-ImplicitCastExpr 'void (*)(const S *)' + // | | `-DeclRefExpr 'void (const S *)' lvalue Function 0x55e4ecbc8208 'useConstPtr' 'void (const S *)' + // | `-ImplicitCastExpr 'const S *' + // | `-UnaryOperator 'S *' prefix '&' cannot overflow + // | `-DeclRefExpr 'S' lvalue ParmVar 0x55e4ecbc82c0 's' 'S' + // + // The only node where constness matters is the `ImplicitCastExpr` to + // `const S*`, because that's the one that is passed to the `CallExpr`. The + // fact that intermediate nodes (e.g. `UnaryOperator`) can modify the value is + // irrelevant, because no ancestor of these nodes actually observes their + // value in a mutable way. + // + // We classify nodes in two types depending on whether they can modify the + // state of the objects represented by their arguments: We call "vetted" the + // nodes that cannot change the that state. + // For example, a `CallExpr` node can mutate the state of its arguments + // (e.g., a function that takes a parameter by reference can mutate the state + // of that argument), so it is not vetted. However, an `LValueToRValue` cast + // does not directly change the state of its argument, (only a parent can) so + // it is vetted. + // + // The algorithm works as follows: we look at the inverse tree rooted at + // `Node` (DeclRefExpr->UnaryOperator->ImplicitCastExpr->CallExpr in the + // example), and walk up the vetted nodes to find nodes that have unvetted + // parents. We call these "unsafe" nodes. + // In the example above, the DeclRefExpr, UnaryOperator, and ImplicitCastExpr + // are vetted. The DeclRefExpr and UnaryOperator are safe, and the + // ImplicitCastExpr is unsafe. + // When we find an unsafe node, we finally check whether the type of that node + // allows mutations (in which case the parent can actually mutate the state), + // or not (e.g. a `const T*` rvalue cannot be modified through that pointer). + // If we find an unsafe node with a type that allows mutation, we cannot + // guarantee the the value is used only in a const way, so we return false. + // Else, we keep traversing the tree until we find a mutable unsafe node or + // there are no more nodes, in which case we conclude that there is no way to + // modify the state of the root. + + // We traverse the inverse tree in DFS. The stack contains only vetted nodes. + std::vector Stack = {&Node}; + auto &Ctx = Finder->getASTContext(); + + // Returns true if the node is vetted. + 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 IsUnsafe = 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 { + // `E` has an unvetted parent, it's unsafe. + IsUnsafe = true; + } + } + if (IsUnsafe) { + // The node is unsafe, check the type of the node for mutability. + 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; + // Not sure about these, err on the safe side. + 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; + } + } + } + } + // All unsafe nodes have immutable type. + return true; +} + } // namespace // Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl @@ -44,44 +186,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/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp --- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp @@ -101,23 +101,23 @@ void f(const S target) { useVal(/*const*/target); useConstRef(/*const*/target); - useConstPtr(&target); - useConstPtrConstRef(&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)target; - (void)⌖ - (void)*⌖ + (void)/*const*/target; + (void)&/*const*/target; + (void)*&/*const*/target; S copy1 = /*const*/target; S copy2(/*const*/target); - useInt(target.int_member); - useIntConstRef(target.int_member); + useInt(/*const*/target.int_member); + useIntConstRef(/*const*/target.int_member); useIntPtr(target.ptr_member); - useIntConstPtr(&target.int_member); + useIntConstPtr(&/*const*/target.int_member); } )"); } @@ -127,23 +127,23 @@ void f(const S& target) { useVal(/*const*/target); useConstRef(/*const*/target); - useConstPtr(&target); - useConstPtrConstRef(&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)target; - (void)⌖ - (void)*⌖ + (void)/*const*/target; + (void)&/*const*/target; + (void)*&/*const*/target; S copy1 = /*const*/target; S copy2(/*const*/target); - useInt(target.int_member); - useIntConstRef(target.int_member); + useInt(/*const*/target.int_member); + useIntConstRef(/*const*/target.int_member); useIntPtr(target.ptr_member); - useIntConstPtr(&target.int_member); + useIntConstPtr(&/*const*/target.int_member); } )"); } @@ -153,8 +153,8 @@ void f(S target, const S& other) { useConstRef(/*const*/target); useVal(/*const*/target); - useConstPtr(&target); - useConstPtrConstRef(&target); + useConstPtr(&/*const*/target); + useConstPtrConstRef(&/*const*/target); /*const*/target.constMethod(); target.nonConstMethod(); /*const*/target(ConstTag{}); @@ -167,15 +167,15 @@ (/*const*/target).constMethod(); (void)(/*const*/target == /*const*/target); (void)(/*const*/target == other); - (void)target; - (void)⌖ - (void)*⌖ + (void)/*const*/target; + (void)&/*const*/target; + (void)*&/*const*/target; S copy1 = /*const*/target; S copy2(/*const*/target); - useInt(target.int_member); - useIntConstRef(target.int_member); + useInt(/*const*/target.int_member); + useIntConstRef(/*const*/target.int_member); useIntPtr(target.ptr_member); - useIntConstPtr(&target.int_member); + useIntConstPtr(&/*const*/target.int_member); } )"); } @@ -185,8 +185,8 @@ void f(S& target) { useVal(/*const*/target); useConstRef(/*const*/target); - useConstPtr(&target); - useConstPtrConstRef(&target); + useConstPtr(&/*const*/target); + useConstPtrConstRef(&/*const*/target); /*const*/target.constMethod(); target.nonConstMethod(); /*const*/target(ConstTag{}); @@ -194,15 +194,15 @@ useConstRef((/*const*/target)); (/*const*/target).constMethod(); (void)(/*const*/target == /*const*/target); - (void)target; - (void)⌖ - (void)*⌖ + (void)/*const*/target; + (void)&/*const*/target; + (void)*&/*const*/target; S copy1 = /*const*/target; S copy2(/*const*/target); - useInt(target.int_member); - useIntConstRef(target.int_member); + useInt(/*const*/target.int_member); + useIntConstRef(/*const*/target.int_member); useIntPtr(target.ptr_member); - useIntConstPtr(&target.int_member); + useIntConstPtr(&/*const*/target.int_member); } )"); } @@ -210,26 +210,26 @@ TEST(ConstReferenceDeclRefExprsTest, PtrVar) { RunTest(R"( void f(S* target) { - useVal(*target); - useConstRef(*target); - useConstPtr(target); + useVal(*/*const*/target); + useConstRef(*/*const*/target); + useConstPtr(/*const*/target); useConstPtrConstRef(/*const*/target); /*const*/target->constMethod(); target->nonConstMethod(); - (*target)(ConstTag{}); + (*/*const*/target)(ConstTag{}); (*target)[42]; target->operator[](42); - useConstRef((*target)); + useConstRef((*/*const*/target)); (/*const*/target)->constMethod(); - (void)(*target == *target); - (void)*target; - (void)target; - S copy1 = *target; - S copy2(*target); - useInt(target->int_member); - useIntConstRef(target->int_member); + (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(&target->int_member); + useIntConstPtr(&/*const*/target->int_member); } )"); } @@ -237,27 +237,27 @@ TEST(ConstReferenceDeclRefExprsTest, ConstPtrVar) { RunTest(R"( void f(const S* target) { - useVal(*target); - useConstRef(*target); - useConstPtr(target); + useVal(*/*const*/target); + useConstRef(*/*const*/target); + useConstPtr(/*const*/target); useConstPtrRef(target); useConstPtrPtr(&target); - useConstPtrConstPtr(&target); + useConstPtrConstPtr(&/*const*/target); useConstPtrConstRef(/*const*/target); /*const*/target->constMethod(); - (*target)(ConstTag{}); - (*target)[42]; + (*/*const*/target)(ConstTag{}); + (*/*const*/target)[42]; /*const*/target->operator[](42); - (void)(*target == *target); - (void)target; - (void)*target; - if(target) {} - S copy1 = *target; - S copy2(*target); - useInt(target->int_member); - useIntConstRef(target->int_member); + (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(&target->int_member); + useIntConstPtr(&/*const*/target->int_member); } )"); } @@ -265,24 +265,24 @@ TEST(ConstReferenceDeclRefExprsTest, ConstPtrPtrVar) { RunTest(R"( void f(const S** target) { - useVal(**target); - useConstRef(**target); - useConstPtr(*target); + useVal(**/*const*/target); + useConstRef(**/*const*/target); + useConstPtr(*/*const*/target); useConstPtrRef(*target); useConstPtrPtr(target); - useConstPtrConstPtr(target); - useConstPtrConstRef(*target); - (void)target; - (void)*target; - (void)**target; - if(target) {} - if(*target) {} - S copy1 = **target; - S copy2(**target); - useInt((*target)->int_member); - useIntConstRef((*target)->int_member); + 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(&(*target)->int_member); + useIntConstPtr(&(*/*const*/target)->int_member); } )"); } @@ -290,22 +290,22 @@ TEST(ConstReferenceDeclRefExprsTest, ConstPtrConstPtrVar) { RunTest(R"( void f(const S* const* target) { - useVal(**target); - useConstRef(**target); - useConstPtr(*target); - useConstPtrConstPtr(target); - useConstPtrConstRef(*target); - (void)target; - (void)target; - (void)**target; - if(target) {} - if(*target) {} - S copy1 = **target; - S copy2(**target); - useInt((*target)->int_member); - useIntConstRef((*target)->int_member); + 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(&(*target)->int_member); + useIntConstPtr(&(*/*const*/target)->int_member); } )"); }