[clang-tidy] Add support for determining constness of more expressions. #82617
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…table. All data is derived from a single table rather than being spread out over an enum, a table and the main entry point.
… with that of the enum.
|
@llvm/pr-subscribers-tools-llvm-exegesis @llvm/pr-subscribers-clang-tools-extra Author: Clement Courbet (legrosbuffle) ChangesThis uses a more systematic approach for determining whcich This allows us to handle most address taking and dereference, bindings to value and const& variables, and track constness of pointee (see changes in DeclRefExprUtilsTest.cpp). This allows supporting more patterns in Those two patterns are relatively common: and In our codebase, we have around 25% additional findings from Patch is 31.94 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82617.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
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 OnExpr = anyOf(
+ // Direct reference to `*this`: `a.f()` or `a->f()`.
+ ReceiverExpr,
+ // Access through dereference, typically used for `operator[]`: `(*a)[3]`.
+ unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
const auto ReceiverType =
hasCanonicalType(recordType(hasDeclaration(namedDecl(
unless(matchers::matchesAnyListedName(ExcludedContainerTypes))))));
- return expr(anyOf(
- cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
- thisPointerType(ReceiverType)),
- cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
- hasArgument(0, hasType(ReceiverType)))));
+ return expr(
+ anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+ thisPointerType(ReceiverType)),
+ cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+ hasArgument(0, hasType(ReceiverType)))));
}
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
static bool isInitializingVariableImmutable(
const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context,
const std::vector<StringRef> &ExcludedContainerTypes) {
- if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+ QualType T = InitializingVar.getType().getCanonicalType();
+ if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
return false;
- QualType T = InitializingVar.getType().getCanonicalType();
// The variable is a value type and we know it is only used as const. Safe
// to reference it and avoid the copy.
if (!isa<ReferenceType, PointerType>(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, *Result.Context);
const bool IsVarOnlyUsedAsConst =
- isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+ isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
+ // `NewVar` is always of non-pointer type.
+ 0);
const CheckContext Context{
NewVar, BlockStmt, VarDeclStmt, *Result.Context,
IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 2d73179150e8b8..663691c519b8e9 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -10,7 +10,9 @@
#include "Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <cassert>
namespace clang::tidy::utils::decl_ref_expr {
@@ -34,69 +36,185 @@ void extractNodesByIdTo(ArrayRef<BoundNodes> Matches, StringRef ID,
Nodes.insert(Match.getNodeAs<Node>(ID));
}
+// A matcher that matches DeclRefExprs that are used in ways such that the
+// underlying declaration is not modified.
+// If the declaration is of pointer type, `Indirections` specifies the level
+// of indirection of the object whose mutations we are tracking.
+//
+// For example, given:
+// ```
+// int i;
+// int* p;
+// p = &i; // (A)
+// *p = 3; // (B)
+// ```
+//
+// `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(0))` matches
+// (B), but `declRefExpr(to(varDecl(hasName("p"))), doesNotMutateObject(1))`
+// matches (A).
+//
+AST_MATCHER_P(DeclRefExpr, doesNotMutateObject, int, Indirections) {
+ // We walk up the parents of the DeclRefExpr recursively until we end up on a
+ // parent that cannot modify the underlying object. There are a few kinds of
+ // expressions:
+ // - Those that cannot be used to mutate the underlying object. We can stop
+ // recursion there.
+ // - Those that can be used to mutate the underlying object in analyzable
+ // ways (such as taking the address or accessing a subobject). We have to
+ // examine the parents.
+ // - Those that we don't know how to analyze. In that case we stop there and
+ // we assume that they can mutate the underlying expression.
+
+ struct StackEntry {
+ StackEntry(const Expr *E, int Indirections)
+ : E(E), Indirections(Indirections) {}
+ // The expression to analyze.
+ const Expr *E;
+ // The number of pointer indirections of the object being tracked (how
+ // many times an address was taken).
+ int Indirections;
+ };
+
+ llvm::SmallVector<StackEntry, 4> Stack;
+ Stack.emplace_back(&Node, Indirections);
+ auto &Ctx = Finder->getASTContext();
+
+ while (!Stack.empty()) {
+ const StackEntry Entry = Stack.back();
+ Stack.pop_back();
+
+ // If the expression type is const-qualified at the appropriate indirection
+ // level then we can not mutate the object.
+ QualType Ty = Entry.E->getType().getCanonicalType();
+ for (int I = 0; I < Entry.Indirections; ++I) {
+ assert(Ty->isPointerType());
+ Ty = Ty->getPointeeType().getCanonicalType();
+ }
+ if (Ty.isConstQualified()) {
+ continue;
+ }
+
+ // Otherwise we have to look at the parents to see how the expression is
+ // used.
+ const auto Parents = Ctx.getParents(*Entry.E);
+ // Note: most nodes have a single parents, but there exist nodes that have
+ // several parents, such as `InitListExpr` that have semantic and syntactic
+ // forms.
+ for (const auto &Parent : Parents) {
+ if (Parent.get<CompoundStmt>()) {
+ // Unused block-scope statement.
+ continue;
+ }
+ const Expr *const P = Parent.get<Expr>();
+ if (P == nullptr) {
+ // `Parent` is not an expr (e.g. a `VarDecl`).
+ // The case of binding to a `const&` or `const*` variable is handled by
+ // the fact that there is going to be a `NoOp` cast to const below the
+ // `VarDecl`, so we're not even going to get there.
+ // The case of copying into a value-typed variable is handled by the
+ // rvalue cast.
+ // This triggers only when binding to a mutable reference/ptr variable.
+ // FIXME: When we take a mutable reference we could keep checking the
+ // new variable for const usage only.
+ return false;
+ }
+ // Cosmetic nodes.
+ if (isa<ParenExpr>(P) || isa<MaterializeTemporaryExpr>(P)) {
+ Stack.emplace_back(P, Entry.Indirections);
+ continue;
+ }
+ if (const auto *const Cast = dyn_cast<CastExpr>(P)) {
+ switch (Cast->getCastKind()) {
+ // NoOp casts are used to add `const`. We'll check whether adding that
+ // const prevents modification when we process the cast.
+ case CK_NoOp:
+ // These do nothing w.r.t. to mutability.
+ case CK_BaseToDerived:
+ case CK_DerivedToBase:
+ case CK_UncheckedDerivedToBase:
+ case CK_Dynamic:
+ case CK_BaseToDerivedMemberPointer:
+ case CK_DerivedToBaseMemberPointer:
+ Stack.emplace_back(Cast, Entry.Indirections);
+ continue;
+ case CK_ToVoid:
+ case CK_PointerToBoolean:
+ // These do not mutate the underlying variable.
+ continue;
+ case CK_LValueToRValue: {
+ // An rvalue is immutable.
+ if (Entry.Indirections == 0) {
+ continue;
+ }
+ Stack.emplace_back(Cast, Entry.Indirections);
+ continue;
+ }
+ default:
+ // Bail out on casts that we cannot analyze.
+ return false;
+ }
+ }
+ if (const auto *const Member = dyn_cast<MemberExpr>(P)) {
+ if (const auto *const Method =
+ dyn_cast<CXXMethodDecl>(Member->getMemberDecl())) {
+ if (!Method->isConst()) {
+ // The method can mutate our variable.
+ return false;
+ }
+ continue;
+ }
+ Stack.emplace_back(Member, 0);
+ continue;
+ }
+ if (const auto *const Op = dyn_cast<UnaryOperator>(P)) {
+ switch (Op->getOpcode()) {
+ case UO_AddrOf:
+ Stack.emplace_back(Op, Entry.Indirections + 1);
+ continue;
+ case UO_Deref:
+ assert(Entry.Indirections > 0);
+ Stack.emplace_back(Op, Entry.Indirections - 1);
+ continue;
+ default:
+ // Bail out on unary operators that we cannot analyze.
+ return false;
+ }
+ }
+
+ // Assume any other expression can modify the underlying variable.
+ return false;
+ }
+ }
+
+ // No parent can modify the variable.
+ return true;
+}
+
} // namespace
-// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
-// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
- ASTContext &Context) {
- auto DeclRefToVar =
- declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
- auto MemberExprOfVar = memberExpr(hasObjectExpression(DeclRefToVar));
- auto DeclRefToVarOrMemberExprOfVar =
- stmt(anyOf(DeclRefToVar, MemberExprOfVar));
- 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(DeclRefToVarOrMemberExprOfVar)),
- cxxOperatorCallExpr(ConstMethodCallee,
- hasArgument(0, DeclRefToVarOrMemberExprOfVar))))),
- Stmt, Context);
+ ASTContext &Context, int Indirections) {
+ auto Matches = match(findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl))),
+ doesNotMutateObject(Indirections))
+ .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(
- DeclRefToVarOrMemberExprOfVar,
- 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(DeclRefToVarOrMemberExprOfVar)))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- Matches = match(findAll(declStmt(has(varDecl(
- hasType(qualType(matchers::isPointerToConst())),
- hasInitializer(ignoringImpCasts(unaryOperator(
- hasOperatorName("&"),
- hasUnaryOperand(DeclRefToVarOrMemberExprOfVar)))))))),
- Stmt, Context);
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
+
return DeclRefs;
}
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
- ASTContext &Context) {
+ ASTContext &Context, int Indirections) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
// CXXConstructExprs where the loop variable is used as argument to a const
// reference parameter.
// If the difference is empty it is safe for the loop variable to be a const
// reference.
auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
- auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
+ auto ConstReferenceDeclRefs =
+ constReferenceDeclRefExprs(Var, Stmt, Context, Indirections);
return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
}
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
index 2c16d95408cf68..8361b9d89ed268 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
@@ -15,15 +15,6 @@
namespace clang::tidy::utils::decl_ref_expr {
-/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
-/// do not modify it.
-///
-/// Returns ``true`` if only const methods or operators are called on the
-/// variable or the variable is a const reference or value argument to a
-/// ``callExpr()``.
-bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
- ASTContext &Context);
-
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
@@ -34,9 +25,31 @@ allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
+///
+/// If ``VarDecl`` is of pointer type, ``Indirections`` specifies the level
+/// of indirection of the object whose mutations we are tracking.
+///
+/// For example, given:
+/// ```
+/// int i;
+/// int* p;
+/// p = &i; // (A)
+/// *p = 3; // (B)
+/// ```
+///
+/// - `constReferenceDeclRefExprs(P, Stmt, Context, 0)` returns the reference
+// to `p` in (B): the pointee is modified, but the pointer is not;
+/// - `constReferenceDeclRefExprs(P, Stmt, Context, 1)` returns the reference
+// to `p` in (A): the pointee is modified, but the pointer is not;
llvm::SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
- ASTContext &Context);
+ ASTContext &Context, int Indirections);
+
+/// Returns true if all ``DeclRefExpr`` to the variable within ``Stmt``
+/// do not modify it.
+/// See `constReferenceDeclRefExprs` for the meaning of ``Indirections``.
+bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
+ ASTContext &Context, int Indirections);
/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor
/// call expression within ``Decl``.
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
index 049ba64d6aedeb..92625cc1332e28 100644
--- 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
@@ -41,6 +41,10 @@ struct Container {
ConstIterator<T> end() const;
void nonConstMethod();
bool constMethod() const;
+
+ const T& at(int) const;
+ T& at(int);
+
};
using ExpensiveToCopyContainerAlias = Container<ExpensiveToCopyType>;
@@ -225,25 +229,25 @@ void PositiveOperatorCallConstValueParamAlias(const ExpensiveToCopyContainerAlia
VarCopyConstructed.constMethod();
}
-void PositiveOperatorCallConstValueParam(const Container<ExpensiveToCopyType>* C) {
+void PositiveOperatorCallConstPtrParam(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);
+ const ExpensiveToCopyType VarAssigned = C->at(42);
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarAssigned'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarAssigned = C->at(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));
+ const ExpensiveToCopyType VarCopyConstructed(C->at(42));
+ // CHECK-MESSAGES: [[@LINE-1]]:29: warning: the const qualified variable 'VarCopyConstructed'
+ // CHECK-FIXES: const ExpensiveToCopyType& VarCopyConstructed(C->at(42));
VarCopyConstructed.constMethod();
}
@@ -876,3 +880,4 @@ void negativeNonConstMemberExpr() {
mutate(&Copy.Member);
}
}
+
diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
index a653b11faad282..4c9e81ea0f61ac 100644
--- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -12,6 +12,7 @@ namespace tidy {
namespace {
using namespace clang::ast_matchers;
+template <int Indirections>
class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
public:
ConstReferenceDeclRefExprsTransform(StringRef CheckName,
@@ -27,7 +28,7 @@ class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
using utils::decl_ref_expr::constReferenceDeclRefExprs;
const auto const_decrefexprs = constReferenceDeclRefExprs(
*D, *cast<FunctionDecl>(D->getDeclContext())->getBody(),
- *Result.Context);
+ *Result.Context, Indirections);
for (const DeclRefExpr *const Expr : const_decrefexprs) {
assert(Expr);
@@ -40,7 +41,7 @@ class ConstReferenceDeclRefExprsTransform : public ClangTidyCheck {
namespace test {
-void RunTest(StringRef Snippet) {
+template <int Indirections> void RunTest(StringRef Snippet) {
StringRef CommonCode = R"(
struct ConstTag{};
@@ -59,6 +60,9 @@ void RunTest(StringRef Snippet) {
bool operator==(const S&) const;
int int_member;
+ // We consider a mutation of the `*ptr_member` to be a const use of
+ // `*this`. This is consistent with the semantics of `const`-qualified
+ // methods, which prevent modifying `ptr_member` but not `*ptr_member`.
int* ptr_member;
};
@@ -92,69 +...
[truncated]
|
| // Unused block-scope statement. | ||
| continue; | ||
| } | ||
| const Expr *const P = Parent.get<Expr>(); |
There was a problem hiding this comment.
auto could be used here - type is explicitly stated.
|
|
||
| // Otherwise we have to look at the parents to see how the expression is | ||
| // used. | ||
| const auto Parents = Ctx.getParents(*Entry.E); |
There was a problem hiding this comment.
Please don't use auto - type is not spelled.
fberger
approved these changes
Feb 23, 2024
PiotrZSL
approved these changes
Feb 23, 2024
legrosbuffle
force-pushed
the
better-isonlyusedasconst
branch
from
badf08d
to
b2b98e9
Compare
This uses a more systematic approach for determining whcich `DeclRefExpr`s mutate the underlying object: Instead of using a few matchers, we walk up the AST until we find a parent that we can prove cannot change the underlying object. This allows us to handle most address taking and dereference, bindings to value and const& variables, and track constness of pointee (see changes in DeclRefExprUtilsTest.cpp). This allows supporting more patterns in `performance-unnecessary-copy-initialization`. Those two patterns are relatively common: ``` const auto e = (*vector_ptr)[i] ``` and ``` const auto e = vector_ptr->at(i); ``` In our codebase, we have around 25% additional findings from `performance-unnecessary-copy-initialization` with this change. I did not see any additional false positives.
legrosbuffle
force-pushed
the
better-isonlyusedasconst
branch
from
cd2052a
to
4a6aaf6
Compare
|
Thanks all. Comments addressed.
Sorry, this was not very clear. By "our codebase" I meant the whole of Google code :) This also ran on numerous third party repos. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses a more systematic approach for determining whcich
DeclRefExprs mutate the underlying object: Instead of using a few matchers, we walk up the AST until we find a parent that we can prove cannot change the underlying object.This allows us to handle most address taking and dereference, bindings to value and const& variables, and track constness of pointee (see changes in DeclRefExprUtilsTest.cpp).
This allows supporting more patterns in
performance-unnecessary-copy-initialization.Those two patterns are relatively common:
and
In our codebase, we have around 25% additional findings from
performance-unnecessary-copy-initializationwith this change. I did not see any additional false positives.