courbet updated this revision to Diff 389504.
courbet added a comment.
Rebase on submitted unit tests so that we can see the changes better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114539/new/
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/DeclRefExprUtilsTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ 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);
}
)");
}
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
