[clang-tools-extra] cb4c12b - [clang-tidy] performance-unnecessary-copy-initialization: Create option to exclude container types from triggering the check.
Felix Berger via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 22 13:21:09 PDT 2021
Author: Felix Berger
Date: 2021-07-22T16:20:20-04:00
New Revision: cb4c12b6117a6f2989d5745854a94c75cb6a09ba
URL: https://github.com/llvm/llvm-project/commit/cb4c12b6117a6f2989d5745854a94c75cb6a09ba
DIFF: https://github.com/llvm/llvm-project/commit/cb4c12b6117a6f2989d5745854a94c75cb6a09ba.diff
LOG: [clang-tidy] performance-unnecessary-copy-initialization: Create option to exclude container types from triggering the check.
Add string list option of type names analagous to `AllowedTypes` which lets
users specify a list of ExcludedContainerTypes.
Types matching this list will not trigger the check when an expensive variable
is copy initialized from a const accessor method they provide, i.e.:
```
ExcludedContainerTypes = 'ExcludedType'
void foo() {
ExcludedType<ExpensiveToCopy> Container;
const ExpensiveToCopy NecessaryCopy = Container.get();
}
```
Even though an expensive to copy variable is copy initialized the check does not
trigger because the container type is excluded.
This is useful for container types that don't own their data, such as view types
where modification of the returned references in other places cannot be reliably
tracked, or const incorrect types.
Differential Revision: https://reviews.llvm.org/D106173
Reviewed-by: ymandel
Added:
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
Modified:
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 9631e06a8f95b..6f9495fd1e66c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -75,7 +75,8 @@ void recordRemoval(const DeclStmt &Stmt, ASTContext &Context,
}
}
-AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
+AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
+ std::vector<std::string>, ExcludedContainerTypes) {
// Match method call expressions where the `this` argument is only used as
// const, this will be checked in `check()` part. This returned const
// reference is highly likely to outlive the local const reference of the
@@ -85,7 +86,11 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningMethodCall) {
return cxxMemberCallExpr(
callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))
.bind(MethodDeclId)),
- on(declRefExpr(to(varDecl().bind(ObjectArgId)))));
+ on(declRefExpr(to(
+ varDecl(
+ unless(hasType(qualType(hasCanonicalType(hasDeclaration(namedDecl(
+ matchers::matchesAnyListedName(ExcludedContainerTypes))))))))
+ .bind(ObjectArgId)))));
}
AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -98,11 +103,13 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
.bind(InitFunctionCallId);
}
-AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) {
+AST_MATCHER_FUNCTION_P(StatementMatcher, initializerReturnsReferenceToConst,
+ std::vector<std::string>, ExcludedContainerTypes) {
auto OldVarDeclRef =
declRefExpr(to(varDecl(hasLocalStorage()).bind(OldVarDeclId)));
return expr(
- anyOf(isConstRefReturningFunctionCall(), isConstRefReturningMethodCall(),
+ anyOf(isConstRefReturningFunctionCall(),
+ isConstRefReturningMethodCall(ExcludedContainerTypes),
ignoringImpCasts(OldVarDeclRef),
ignoringImpCasts(unaryOperator(hasOperatorName("&"),
hasUnaryOperand(OldVarDeclRef)))));
@@ -120,9 +127,9 @@ AST_MATCHER_FUNCTION(StatementMatcher, initializerReturnsReferenceToConst) {
// the same set of criteria we apply when identifying the unnecessary copied
// variable in this check to begin with. In this case we check whether the
// object arg or variable that is referenced is immutable as well.
-static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
- const Stmt &BlockStmt,
- ASTContext &Context) {
+static bool isInitializingVariableImmutable(
+ const VarDecl &InitializingVar, const Stmt &BlockStmt, ASTContext &Context,
+ const std::vector<std::string> &ExcludedContainerTypes) {
if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
return false;
@@ -138,18 +145,21 @@ static bool isInitializingVariableImmutable(const VarDecl &InitializingVar,
return true;
}
- auto Matches = match(initializerReturnsReferenceToConst(),
- *InitializingVar.getInit(), Context);
+ auto Matches =
+ match(initializerReturnsReferenceToConst(ExcludedContainerTypes),
+ *InitializingVar.getInit(), Context);
// The reference is initialized from a free function without arguments
// returning a const reference. This is a global immutable object.
if (selectFirst<CallExpr>(InitFunctionCallId, Matches) != nullptr)
return true;
// Check that the object argument is immutable as well.
if (const auto *OrigVar = selectFirst<VarDecl>(ObjectArgId, Matches))
- return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+ return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context,
+ ExcludedContainerTypes);
// Check that the old variable we reference is immutable as well.
if (const auto *OrigVar = selectFirst<VarDecl>(OldVarDeclId, Matches))
- return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context);
+ return isInitializingVariableImmutable(*OrigVar, BlockStmt, Context,
+ ExcludedContainerTypes);
return false;
}
@@ -204,7 +214,9 @@ UnnecessaryCopyInitialization::UnnecessaryCopyInitialization(
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
AllowedTypes(
- utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
+ utils::options::parseStringList(Options.get("AllowedTypes", ""))),
+ ExcludedContainerTypes(utils::options::parseStringList(
+ Options.get("ExcludedContainerTypes", ""))) {}
void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
auto LocalVarCopiedFrom = [this](const internal::Matcher<Expr> &CopyCtorArg) {
@@ -235,7 +247,8 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
};
Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
- isConstRefReturningMethodCall())),
+ isConstRefReturningMethodCall(
+ ExcludedContainerTypes))),
this);
Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
@@ -291,7 +304,8 @@ void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
return;
if (ObjectArg != nullptr &&
- !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context))
+ !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
+ ExcludedContainerTypes))
return;
if (isVariableUnused(Var, BlockStmt, Context)) {
auto Diagnostic =
@@ -318,7 +332,8 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
const VarDecl &NewVar, const VarDecl &OldVar, const Stmt &BlockStmt,
const DeclStmt &Stmt, bool IssueFix, ASTContext &Context) {
if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
- !isInitializingVariableImmutable(OldVar, BlockStmt, Context))
+ !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+ ExcludedContainerTypes))
return;
if (isVariableUnused(NewVar, BlockStmt, Context)) {
@@ -344,6 +359,8 @@ void UnnecessaryCopyInitialization::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
+ Options.store(Opts, "ExcludedContainerTypes",
+ utils::options::serializeStringList(ExcludedContainerTypes));
}
} // namespace performance
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index c11be2d8885d6..d81a89aa3e559 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -43,6 +43,7 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
const Stmt &BlockStmt, const DeclStmt &Stmt,
bool IssueFix, ASTContext &Context);
const std::vector<std::string> AllowedTypes;
+ const std::vector<std::string> ExcludedContainerTypes;
};
} // namespace performance
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst b/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
index 7a8fe8408f422..837283811ddcc 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
@@ -47,3 +47,15 @@ Options
is empty. If a name in the list contains the sequence `::` it is matched
against the qualified typename (i.e. `namespace::Type`, otherwise it is
matched against only the type name (i.e. `Type`).
+
+.. option:: ExcludedContainerTypes
+
+ A semicolon-separated list of names of types whose methods are allowed to
+ return the const reference the variable is copied from. When an expensive to
+ copy variable is copy initialized by the return value from a type on this
+ list the check does not trigger. This can be used to exclude types known to
+ be const incorrect or where the lifetime or immutability of returned
+ references is not tied to mutations of the container. An example are view
+ types that don't own the underlying data. Like for `AllowedTypes` above,
+ regular expressions are accepted and the inclusion of `::` determines whether
+ the qualified typename is matched or not.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
new file mode 100644
index 0000000000000..6d1d28ad14985
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t -- -config="{CheckOptions: [{key: performance-unnecessary-copy-initialization.ExcludedContainerTypes, value: 'ns::ViewType$;::ConstInCorrectType$'}]}" --
+
+namespace ns {
+template <typename T>
+struct ViewType {
+ ViewType(const T &);
+ const T &view() const;
+};
+} // namespace ns
+
+template <typename T>
+struct ViewType {
+ ViewType(const T &);
+ const T &view() const;
+};
+
+struct ExpensiveToCopy {
+ ~ExpensiveToCopy();
+ void constMethod() const;
+};
+
+struct ConstInCorrectType {
+ const ExpensiveToCopy &secretlyMutates() const;
+};
+
+using NSVTE = ns::ViewType<ExpensiveToCopy>;
+typedef ns::ViewType<ExpensiveToCopy> FewType;
+
+void positiveViewType() {
+ ExpensiveToCopy E;
+ ViewType<ExpensiveToCopy> V(E);
+ const auto O = V.view();
+ // CHECK-MESSAGES: [[@LINE-1]]:14: warning: the const qualified variable 'O' is copy-constructed
+ // CHECK-FIXES: const auto& O = V.view();
+ O.constMethod();
+}
+
+void excludedViewTypeInNamespace() {
+ ExpensiveToCopy E;
+ ns::ViewType<ExpensiveToCopy> V(E);
+ const auto O = V.view();
+ O.constMethod();
+}
+
+void excludedViewTypeAliased() {
+ ExpensiveToCopy E;
+ NSVTE V(E);
+ const auto O = V.view();
+ O.constMethod();
+
+ FewType F(E);
+ const auto P = F.view();
+ P.constMethod();
+}
+
+void excludedConstIncorrectType() {
+ ConstInCorrectType C;
+ const auto E = C.secretlyMutates();
+ E.constMethod();
+}
More information about the cfe-commits
mailing list