flx updated this revision to Diff 360865.
flx added a comment.
Add test file for the new option.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106173/new/
https://reviews.llvm.org/D106173
Files:
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
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization-excluded-container-types.cpp
===================================================================
--- /dev/null
+++ 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();
+}
Index: clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
+++ clang-tools-extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
@@ -47,3 +47,15 @@
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.
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -43,6 +43,7 @@
const Stmt &BlockStmt, const DeclStmt &Stmt,
bool IssueFix, ASTContext &Context);
const std::vector<std::string> AllowedTypes;
+ const std::vector<std::string> ExcludedContainerTypes;
};
} // namespace performance
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
@@ -73,7 +73,8 @@
}
}
-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
@@ -82,7 +83,11 @@
// called object.
return cxxMemberCallExpr(
callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))),
- 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) {
@@ -94,11 +99,13 @@
.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)))));
@@ -116,9 +123,9 @@
// 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;
@@ -134,18 +141,21 @@
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;
}
@@ -161,7 +171,9 @@
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) {
@@ -192,7 +204,8 @@
};
Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
- isConstRefReturningMethodCall())),
+ isConstRefReturningMethodCall(
+ ExcludedContainerTypes))),
this);
Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
@@ -238,7 +251,8 @@
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 =
@@ -265,7 +279,8 @@
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)) {
@@ -291,6 +306,8 @@
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
+ Options.store(Opts, "ExcludedContainerTypes",
+ utils::options::serializeStringList(ExcludedContainerTypes));
}
} // namespace performance
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
