Skip to content

Commit

Permalink
[clang-tidy] performance-unnecessary-copy-initialization: Create opti…
Browse files Browse the repository at this point in the history
…on 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
  • Loading branch information
Felix Berger committed Jul 22, 2021
1 parent 46667a1 commit cb4c12b
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 15 deletions.
Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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)))));
Expand All @@ -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;

Expand All @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -235,7 +247,8 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
};

Finder->addMatcher(LocalVarCopiedFrom(anyOf(isConstRefReturningFunctionCall(),
isConstRefReturningMethodCall())),
isConstRefReturningMethodCall(
ExcludedContainerTypes))),
this);

Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
Expand Down Expand Up @@ -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 =
Expand All @@ -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)) {
Expand All @@ -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
Expand Down
Expand Up @@ -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
Expand Down
Expand Up @@ -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.
@@ -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();
}

0 comments on commit cb4c12b

Please sign in to comment.