flx created this revision.
flx added reviewers: aaron.ballman, ymandel, gribozavr2.
Herald added a subscriber: xazax.hun.
flx requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
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
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 not 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. As 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
