https://github.com/gaul updated https://github.com/llvm/llvm-project/pull/222159
>From e71aea47086c3d326f26dd2c58c1882610a2c71c Mon Sep 17 00:00:00 2001 From: Andrew Gaul <[email protected]> Date: Sun, 1 Nov 2026 16:52:11 -0800 Subject: [PATCH 1/2] [clang-tidy] Move special member queries to utils::type_traits and add unit tests Moves isCopyConstructible, isMoveConstructible, isCopyAssignable and isMoveAssignable from cppcoreguidelines-avoid-const-or-ref-data-members into utils::type_traits so they can be shared, and adds unit tests for them. NFC. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../AvoidConstOrRefDataMembersCheck.cpp | 79 +----------- .../clang-tidy/utils/TypeTraits.cpp | 72 +++++++++++ .../clang-tidy/utils/TypeTraits.h | 14 ++ .../unittests/clang-tidy/TypeTraitsTest.cpp | 120 ++++++++++++++++++ 4 files changed, 211 insertions(+), 74 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp index 78bcc3e7e0eccd..6fdcd490bd1865 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "AvoidConstOrRefDataMembersCheck.h" +#include "../utils/TypeTraits.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -14,78 +15,6 @@ using namespace clang::ast_matchers; namespace clang::tidy::cppcoreguidelines { -static bool isCopyConstructible(const CXXRecordDecl &Node) { - if (Node.needsOverloadResolutionForCopyConstructor() && - Node.needsImplicitCopyConstructor()) { - // unresolved - for (const CXXBaseSpecifier &BS : Node.bases()) { - const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); - if (BRD != nullptr && !isCopyConstructible(*BRD)) - return false; - } - } - if (Node.hasSimpleCopyConstructor()) - return true; - for (const CXXConstructorDecl *Ctor : Node.ctors()) - if (Ctor->isCopyConstructor()) - return !Ctor->isDeleted(); - return false; -} - -static bool isMoveConstructible(const CXXRecordDecl &Node) { - if (Node.needsOverloadResolutionForMoveConstructor() && - Node.needsImplicitMoveConstructor()) { - // unresolved - for (const CXXBaseSpecifier &BS : Node.bases()) { - const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); - if (BRD != nullptr && !isMoveConstructible(*BRD)) - return false; - } - } - if (Node.hasSimpleMoveConstructor()) - return true; - for (const CXXConstructorDecl *Ctor : Node.ctors()) - if (Ctor->isMoveConstructor()) - return !Ctor->isDeleted(); - return false; -} - -static bool isCopyAssignable(const CXXRecordDecl &Node) { - if (Node.needsOverloadResolutionForCopyAssignment() && - Node.needsImplicitCopyAssignment()) { - // unresolved - for (const CXXBaseSpecifier &BS : Node.bases()) { - const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); - if (BRD != nullptr && !isCopyAssignable(*BRD)) - return false; - } - } - if (Node.hasSimpleCopyAssignment()) - return true; - for (const CXXMethodDecl *Method : Node.methods()) - if (Method->isCopyAssignmentOperator()) - return !Method->isDeleted(); - return false; -} - -static bool isMoveAssignable(const CXXRecordDecl &Node) { - if (Node.needsOverloadResolutionForMoveAssignment() && - Node.needsImplicitMoveAssignment()) { - // unresolved - for (const CXXBaseSpecifier &BS : Node.bases()) { - const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); - if (BRD != nullptr && !isMoveAssignable(*BRD)) - return false; - } - } - if (Node.hasSimpleMoveAssignment()) - return true; - for (const CXXMethodDecl *Method : Node.methods()) - if (Method->isMoveAssignmentOperator()) - return !Method->isDeleted(); - return false; -} - namespace { AST_MATCHER(FieldDecl, isMemberOfLambda) { @@ -93,8 +22,10 @@ AST_MATCHER(FieldDecl, isMemberOfLambda) { } AST_MATCHER(CXXRecordDecl, isCopyableOrMovable) { - return isCopyConstructible(Node) || isMoveConstructible(Node) || - isCopyAssignable(Node) || isMoveAssignable(Node); + return utils::type_traits::isCopyConstructible(Node) || + utils::type_traits::isMoveConstructible(Node) || + utils::type_traits::isCopyAssignable(Node) || + utils::type_traits::isMoveAssignable(Node); } } // namespace diff --git a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp index ac2fcf72fef836..4c0e72a716fb12 100644 --- a/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp +++ b/clang-tools-extra/clang-tidy/utils/TypeTraits.cpp @@ -147,6 +147,78 @@ bool hasNonTrivialMoveAssignment(QualType Type) { Record->hasNonTrivialMoveAssignment(); } +bool isCopyConstructible(const CXXRecordDecl &Record) { + if (Record.needsOverloadResolutionForCopyConstructor() && + Record.needsImplicitCopyConstructor()) { + // unresolved + for (const CXXBaseSpecifier &BS : Record.bases()) { + const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); + if (BRD != nullptr && !isCopyConstructible(*BRD)) + return false; + } + } + if (Record.hasSimpleCopyConstructor()) + return true; + for (const CXXConstructorDecl *Ctor : Record.ctors()) + if (Ctor->isCopyConstructor()) + return !Ctor->isDeleted(); + return false; +} + +bool isMoveConstructible(const CXXRecordDecl &Record) { + if (Record.needsOverloadResolutionForMoveConstructor() && + Record.needsImplicitMoveConstructor()) { + // unresolved + for (const CXXBaseSpecifier &BS : Record.bases()) { + const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); + if (BRD != nullptr && !isMoveConstructible(*BRD)) + return false; + } + } + if (Record.hasSimpleMoveConstructor()) + return true; + for (const CXXConstructorDecl *Ctor : Record.ctors()) + if (Ctor->isMoveConstructor()) + return !Ctor->isDeleted(); + return false; +} + +bool isCopyAssignable(const CXXRecordDecl &Record) { + if (Record.needsOverloadResolutionForCopyAssignment() && + Record.needsImplicitCopyAssignment()) { + // unresolved + for (const CXXBaseSpecifier &BS : Record.bases()) { + const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); + if (BRD != nullptr && !isCopyAssignable(*BRD)) + return false; + } + } + if (Record.hasSimpleCopyAssignment()) + return true; + for (const CXXMethodDecl *Method : Record.methods()) + if (Method->isCopyAssignmentOperator()) + return !Method->isDeleted(); + return false; +} + +bool isMoveAssignable(const CXXRecordDecl &Record) { + if (Record.needsOverloadResolutionForMoveAssignment() && + Record.needsImplicitMoveAssignment()) { + // unresolved + for (const CXXBaseSpecifier &BS : Record.bases()) { + const CXXRecordDecl *BRD = BS.getType()->getAsCXXRecordDecl(); + if (BRD != nullptr && !isMoveAssignable(*BRD)) + return false; + } + } + if (Record.hasSimpleMoveAssignment()) + return true; + for (const CXXMethodDecl *Method : Record.methods()) + if (Method->isMoveAssignmentOperator()) + return !Method->isDeleted(); + return false; +} + static bool declIsStdInitializerList(const NamedDecl *D) { return D->isInStdNamespace() && D->getName() == "initializer_list"; } diff --git a/clang-tools-extra/clang-tidy/utils/TypeTraits.h b/clang-tools-extra/clang-tidy/utils/TypeTraits.h index 03711461b51acf..e8a28fb60049cd 100644 --- a/clang-tools-extra/clang-tidy/utils/TypeTraits.h +++ b/clang-tools-extra/clang-tidy/utils/TypeTraits.h @@ -34,6 +34,20 @@ bool hasNonTrivialMoveConstructor(QualType Type); /// Return true if `Type` has a non-trivial move assignment operator. bool hasNonTrivialMoveAssignment(QualType Type); +/// Returns `true` if `Record` has a copy constructor that is not deleted. +bool isCopyConstructible(const CXXRecordDecl &Record); + +/// Returns `true` if `Record` has a move constructor that is not deleted. +bool isMoveConstructible(const CXXRecordDecl &Record); + +/// Returns `true` if `Record` has a copy assignment operator that is not +/// deleted. +bool isCopyAssignable(const CXXRecordDecl &Record); + +/// Returns `true` if `Record` has a move assignment operator that is not +/// deleted. +bool isMoveAssignable(const CXXRecordDecl &Record); + /// Returns `true` if `Type` is a `std::initializer_list<...>` specialization. bool isStdInitializerList(QualType Type); diff --git a/clang-tools-extra/unittests/clang-tidy/TypeTraitsTest.cpp b/clang-tools-extra/unittests/clang-tidy/TypeTraitsTest.cpp index 13261c3aa9e527..ce3d22ba312c45 100644 --- a/clang-tools-extra/unittests/clang-tidy/TypeTraitsTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/TypeTraitsTest.cpp @@ -56,5 +56,125 @@ TEST(IsStdInitializerListTest, RejectsNonStdInitializerList) { "v")); } +// Returns the result of `Predicate` for the definition of class `S` in `Code`. +bool classSatisfies(bool (*Predicate)(const CXXRecordDecl &), StringRef Code) { + std::unique_ptr<ASTUnit> AST = + tooling::buildASTFromCodeWithArgs(Code, {"-std=c++17"}); + EXPECT_NE(AST, nullptr); + auto Matches = match(cxxRecordDecl(hasName("S"), isDefinition()).bind("s"), + AST->getASTContext()); + EXPECT_EQ(Matches.size(), 1u); + return Predicate(*Matches[0].getNodeAs<CXXRecordDecl>("s")); +} + +TEST(IsCopyConstructibleTest, Implicit) { + EXPECT_TRUE( + classSatisfies(utils::type_traits::isCopyConstructible, "struct S {};")); +} + +TEST(IsCopyConstructibleTest, UserDeclared) { + EXPECT_TRUE(classSatisfies(utils::type_traits::isCopyConstructible, + "struct S { S(const S &); };")); +} + +TEST(IsCopyConstructibleTest, Deleted) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyConstructible, + "struct S { S(const S &) = delete; };")); +} + +TEST(IsCopyConstructibleTest, DeletedByMoveConstructor) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyConstructible, + "struct S { S(S &&); };")); +} + +TEST(IsCopyConstructibleTest, DeletedByMember) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyConstructible, + "struct M { M(const M &) = delete; };" + "struct S { M m; };")); +} + +TEST(IsCopyConstructibleTest, DeletedByBase) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyConstructible, + "struct B { B(const B &) = delete; };" + "struct S : B {};")); +} + +TEST(IsMoveConstructibleTest, Implicit) { + EXPECT_TRUE( + classSatisfies(utils::type_traits::isMoveConstructible, "struct S {};")); +} + +TEST(IsMoveConstructibleTest, UserDeclared) { + EXPECT_TRUE(classSatisfies(utils::type_traits::isMoveConstructible, + "struct S { S(S &&); };")); +} + +TEST(IsMoveConstructibleTest, Deleted) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isMoveConstructible, + "struct S { S(S &&) = delete; };")); +} + +TEST(IsMoveConstructibleTest, NotDeclared) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isMoveConstructible, + "struct S { S(const S &); };")); +} + +TEST(IsMoveConstructibleTest, DeletedByMember) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isMoveConstructible, + "struct M { M(M &&) = delete; };" + "struct S { M m; };")); +} + +TEST(IsCopyAssignableTest, Implicit) { + EXPECT_TRUE( + classSatisfies(utils::type_traits::isCopyAssignable, "struct S {};")); +} + +TEST(IsCopyAssignableTest, UserDeclared) { + EXPECT_TRUE(classSatisfies(utils::type_traits::isCopyAssignable, + "struct S { S &operator=(const S &); };")); +} + +TEST(IsCopyAssignableTest, Deleted) { + EXPECT_FALSE( + classSatisfies(utils::type_traits::isCopyAssignable, + "struct S { S &operator=(const S &) = delete; };")); +} + +TEST(IsCopyAssignableTest, DeletedByMoveAssignment) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyAssignable, + "struct S { S &operator=(S &&); };")); +} + +TEST(IsCopyAssignableTest, DeletedByConstMember) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyAssignable, + "struct S { const int I; };")); +} + +TEST(IsCopyAssignableTest, DeletedByReferenceMember) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isCopyAssignable, + "struct S { int &R; };")); +} + +TEST(IsMoveAssignableTest, Implicit) { + EXPECT_TRUE( + classSatisfies(utils::type_traits::isMoveAssignable, "struct S {};")); +} + +TEST(IsMoveAssignableTest, UserDeclared) { + EXPECT_TRUE(classSatisfies(utils::type_traits::isMoveAssignable, + "struct S { S &operator=(S &&); };")); +} + +TEST(IsMoveAssignableTest, Deleted) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isMoveAssignable, + "struct S { S &operator=(S &&) = delete; };")); +} + +TEST(IsMoveAssignableTest, NotDeclared) { + EXPECT_FALSE(classSatisfies(utils::type_traits::isMoveAssignable, + "struct S { S &operator=(const S &); };")); +} + } // namespace } // namespace clang::tidy::test >From 01878487859f546f33697d466525af263986431e Mon Sep 17 00:00:00 2001 From: Andrew Gaul <[email protected]> Date: Sun, 1 Nov 2026 17:07:40 -0800 Subject: [PATCH 2/2] [clang-tidy] Add performance-inefficient-container-assignment check Finds assignments of a temporary container to a container of the same type that could reuse the storage of the destination instead, and rewrites them to `assign`: v = std::vector<int>(n, 0); -> v.assign(n, 0); v = std::vector<int>(first, last); -> v.assign(first, last); v = {first, last}; -> v.assign(first, last); v = std::vector<int>(std::from_range, r); -> v.assign_range(r); s = std::string(p, n); -> s.assign(p, n); The temporary allocates its own buffer and the move assignment then frees the one the destination already owns; `assign` writes into the existing buffer and only allocates when the new contents do not fit. Unlike constructing the temporary, `assign` needs the elements to be assignable from its arguments and must not be given iterators into the destination, so such assignments are not diagnosed. The fix-it is omitted when the value of the assignment is used, because `assign` does not yield the container, and when the arguments and the destination could affect each other: the assignment evaluates its right operand first, whereas a member call evaluates its object first. Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RN4gpjgX8QWXRuQoBzYace --- .../clang-tidy/performance/CMakeLists.txt | 2 + .../InefficientContainerAssignmentCheck.cpp | 423 ++++++++++++++++++ .../InefficientContainerAssignmentCheck.h | 39 ++ .../performance/PerformanceTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.md | 6 + .../docs/clang-tidy/checks/list.md | 1 + .../inefficient-container-assignment.md | 39 ++ .../checkers/Inputs/Headers/std/deque | 1 + .../checkers/Inputs/Headers/std/forward_list | 1 + .../checkers/Inputs/Headers/std/list | 1 + .../checkers/Inputs/Headers/std/string | 3 + .../checkers/Inputs/Headers/std/vector | 20 + ...inefficient-container-assignment-cxx23.cpp | 31 ++ .../inefficient-container-assignment.cpp | 245 ++++++++++ 14 files changed, 815 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-container-assignment.md create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment-cxx23.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment.cpp diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt index f55a6cf2800f36..e322ebda567ca5 100644 --- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt @@ -10,6 +10,7 @@ add_clang_library(clangTidyPerformanceModule STATIC ForRangeCopyCheck.cpp ImplicitConversionInLoopCheck.cpp InefficientAlgorithmCheck.cpp + InefficientContainerAssignmentCheck.cpp InefficientStringConcatenationCheck.cpp InefficientVectorOperationCheck.cpp MoveConstArgCheck.cpp @@ -46,4 +47,5 @@ clang_target_link_libraries(clangTidyPerformanceModule clangBasic clangLex clangTooling + clangTransformer ) diff --git a/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.cpp b/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.cpp new file mode 100644 index 00000000000000..7246760b1b3f19 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.cpp @@ -0,0 +1,423 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "InefficientContainerAssignmentCheck.h" +#include "../utils/OptionsUtils.h" +#include "../utils/TypeTraits.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/DeclTemplate.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/ParentMapContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Tooling/Transformer/SourceCode.h" +#include "clang/Tooling/Transformer/SourceCodeBuilders.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallPtrSet.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" +#include <optional> +#include <string> + +using namespace clang::ast_matchers; + +namespace clang::tidy::performance { + +namespace { + +// The order matches the %select in the diagnostic. +enum class Rewrite { Assign, AssignRange }; + +using DeclSet = llvm::SmallPtrSet<const ValueDecl *, 4>; + +} // namespace + +static bool +anyReferencedDecl(const Stmt &S, + llvm::function_ref<bool(const ValueDecl *)> Pred) { + if (const auto *Ref = dyn_cast<DeclRefExpr>(&S); Ref && Pred(Ref->getDecl())) + return true; + if (const auto *Member = dyn_cast<MemberExpr>(&S); + Member && Pred(Member->getMemberDecl())) + return true; + return llvm::any_of(S.children(), [&](const Stmt *Child) { + return Child && anyReferencedDecl(*Child, Pred); + }); +} + +static DeclSet referencedDecls(const Stmt &S) { + DeclSet Decls; + anyReferencedDecl(S, [&](const ValueDecl *D) { + Decls.insert(D); + return false; + }); + return Decls; +} + +static bool refersToAnyOf(const Stmt &S, const DeclSet &Decls) { + return anyReferencedDecl( + S, [&](const ValueDecl *D) { return Decls.contains(D); }); +} + +// An explicitly passed allocator has no counterpart in 'assign'. It is the +// parameter whose type is a template argument of the container other than the +// element type. +static bool passesAllocator(const CXXConstructorDecl &Ctor, unsigned NumArgs) { + const auto *Spec = + dyn_cast<ClassTemplateSpecializationDecl>(Ctor.getParent()); + if (!Spec) + return false; + const TemplateArgumentList &TemplateArgs = Spec->getTemplateArgs(); + for (unsigned I = 0, E = std::min(NumArgs, Ctor.getNumParams()); I < E; ++I) { + const QualType ParamType = + Ctor.getParamDecl(I)->getType().getNonReferenceType(); + if (!ParamType->isRecordType()) + continue; + for (unsigned J = 1; J < TemplateArgs.size(); ++J) { + const TemplateArgument &Arg = TemplateArgs[J]; + if (Arg.getKind() == TemplateArgument::Type && + ASTContext::hasSameUnqualifiedType(Arg.getAsType(), ParamType)) + return true; + } + } + return false; +} + +static bool isFromRangeTag(QualType T) { + const CXXRecordDecl *RD = T.getNonReferenceType()->getAsCXXRecordDecl(); + return RD && RD->isInStdNamespace() && RD->getName() == "from_range_t"; +} + +static QualType getElementType(QualType Container) { + const auto *Spec = dyn_cast_if_present<ClassTemplateSpecializationDecl>( + Container->getAsCXXRecordDecl()); + if (!Spec || Spec->getTemplateArgs().size() == 0) + return {}; + const TemplateArgument &Arg = Spec->getTemplateArgs()[0]; + return Arg.getKind() == TemplateArgument::Type ? Arg.getAsType() : QualType(); +} + +static const CXXRecordDecl *getDefinition(QualType T) { + const CXXRecordDecl *Record = T->getAsCXXRecordDecl(); + return Record ? Record->getDefinition() : nullptr; +} + +static bool isCopyAssignable(QualType T) { + T = T.getCanonicalType(); + if (T.isConstQualified()) + return false; + if (T->isScalarType()) + return true; + const CXXRecordDecl *Record = getDefinition(T); + return Record && utils::type_traits::isCopyAssignable(*Record); +} + +static const CXXMethodDecl *findNullaryMethod(const CXXRecordDecl &Record, + DeclarationName Name) { + for (const NamedDecl *D : Record.lookup(Name)) + if (const auto *Method = dyn_cast<CXXMethodDecl>(D); + Method && Method->getNumParams() == 0) + return Method; + for (const CXXBaseSpecifier &Base : Record.bases()) + if (const CXXRecordDecl *BaseRecord = getDefinition(Base.getType())) + if (const CXXMethodDecl *Method = findNullaryMethod(*BaseRecord, Name)) + return Method; + return nullptr; +} + +// Returns the type of '*It', or a null type if it is unknown. +static QualType getDereferencedType(QualType It, ASTContext &Ctx) { + It = It.getNonReferenceType(); + if (const auto *Pointer = It->getAs<PointerType>()) + return Pointer->getPointeeType(); + const CXXRecordDecl *Record = getDefinition(It); + const CXXMethodDecl *Deref = + Record ? findNullaryMethod( + *Record, Ctx.DeclarationNames.getCXXOperatorName(OO_Star)) + : nullptr; + return Deref ? Deref->getReturnType() : QualType(); +} + +static QualType getRangeElementType(QualType Range, ASTContext &Ctx) { + Range = Range.getNonReferenceType(); + if (const ArrayType *Array = Ctx.getAsArrayType(Range)) + return Array->getElementType(); + const CXXRecordDecl *Record = getDefinition(Range); + const CXXMethodDecl *Begin = + Record ? findNullaryMethod(*Record, &Ctx.Idents.get("begin")) : nullptr; + return Begin ? getDereferencedType(Begin->getReturnType(), Ctx) : QualType(); +} + +static bool isAssignableFrom(QualType T, QualType Source, + const ASTContext &Ctx) { + if (Source.isNull()) + return false; + T = T.getCanonicalType(); + Source = Source.getNonReferenceType().getCanonicalType().getUnqualifiedType(); + if (T->isScalarType()) + return !T.isConstQualified() && Source->isScalarType(); + if (ASTContext::hasSameUnqualifiedType(T, Source)) + return isCopyAssignable(T); + const CXXRecordDecl *Record = getDefinition(T); + return Record && + llvm::any_of(Record->methods(), [&](const CXXMethodDecl *Method) { + if (Method->getOverloadedOperator() != OO_Equal || + Method->getNumParams() != 1 || Method->isDeleted() || + Method->getAccess() != AS_public) + return false; + const QualType Param = Method->getParamDecl(0) + ->getType() + .getNonReferenceType() + .getCanonicalType() + .getUnqualifiedType(); + if (Param == Source) + return true; + const auto *ParamPointer = Param->getAs<PointerType>(); + const auto *SourcePointer = Source->getAs<PointerType>(); + return ParamPointer && SourcePointer && + ASTContext::hasSameUnqualifiedType( + ParamPointer->getPointeeType(), + SourcePointer->getPointeeType()) && + ParamPointer->getPointeeType().isAtLeastAsQualifiedAs( + SourcePointer->getPointeeType(), Ctx); + }); +} + +// 'assign' needs the elements to be assignable from its arguments, which +// constructing the temporary does not. +static bool canAssign(const CXXConstructorDecl &Ctor, + ArrayRef<const Expr *> Args, QualType Element, + ASTContext &Ctx) { + const bool IteratorPair = + Ctor.getPrimaryTemplate() && Args.size() == 2 && + ASTContext::hasSameType(Ctor.getParamDecl(0)->getType(), + Ctor.getParamDecl(1)->getType()); + if (IteratorPair) + return isAssignableFrom(Element, + getDereferencedType(Args[0]->getType(), Ctx), Ctx); + return isCopyAssignable(Element); +} + +static std::optional<Rewrite> getRewrite(const CXXConstructorDecl &Ctor, + ArrayRef<const Expr *> Args, + const Expr &LHS, QualType Element, + ASTContext &Ctx) { + if (Args.size() < 2) + return std::nullopt; + + // 'assign' must not be given iterators, pointers or elements that refer into + // the container it replaces. + const DeclSet LHSDecls = referencedDecls(LHS); + if (llvm::any_of(Args, [&](const Expr *Arg) { + return !Arg->getType()->isArithmeticType() && + refersToAnyOf(*Arg, LHSDecls); + })) + return std::nullopt; + + if (isFromRangeTag(Ctor.getParamDecl(0)->getType())) { + if (Args.size() != 2 || + !isAssignableFrom(Element, getRangeElementType(Args[1]->getType(), Ctx), + Ctx)) + return std::nullopt; + return Rewrite::AssignRange; + } + if (!canAssign(Ctor, Args, Element, Ctx)) + return std::nullopt; + return Rewrite::Assign; +} + +// Returns true if E designates a local variable, or a member of one, that +// Other does not mention. +static bool isUnmentionedLocal(const Expr &E, const DeclSet &Other) { + const Expr *Base = E.IgnoreParenImpCasts(); + for (const auto *Member = dyn_cast<MemberExpr>(Base); + Member && !Member->isArrow(); Member = dyn_cast<MemberExpr>(Base)) + Base = Member->getBase()->IgnoreParenImpCasts(); + const auto *Ref = dyn_cast<DeclRefExpr>(Base); + const auto *Var = Ref ? dyn_cast<VarDecl>(Ref->getDecl()) : nullptr; + return Var && Var->hasLocalStorage() && !Var->getType()->isReferenceType() && + !Var->getType().isVolatileQualified() && !Other.contains(Var); +} + +static bool readsOnlyUnmentionedLocals(const Stmt &S, const DeclSet &Other) { + if (isa<CallExpr>(S)) + return false; + if (const auto *Cast = dyn_cast<ImplicitCastExpr>(&S); + Cast && Cast->getCastKind() == CK_LValueToRValue) + return isUnmentionedLocal(*Cast->getSubExpr(), Other); + return llvm::all_of(S.children(), [&](const Stmt *Child) { + return !Child || readsOnlyUnmentionedLocals(*Child, Other); + }); +} + +// Arguments bound to reference parameters are read by the callee. +static bool isUnaffectedArgument(const Expr &Arg, const DeclSet &LHSDecls, + const ASTContext &Ctx) { + if (Arg.HasSideEffects(Ctx)) + return false; + const Expr *E = &Arg; + if (const auto *Temporary = dyn_cast<MaterializeTemporaryExpr>(E)) + E = Temporary->getSubExpr(); + if (E->isGLValue() && !isUnmentionedLocal(*E, LHSDecls)) + return false; + return readsOnlyUnmentionedLocals(*E, LHSDecls); +} + +// An assignment evaluates its right operand first (since C++17), whereas a +// member call evaluates its object expression first. The rewrite is safe when +// neither side can affect the other. +static bool isEvaluationOrderIndependent(const Expr &LHS, + ArrayRef<const Expr *> Args, + ASTContext &Ctx) { + DeclSet ArgDecls; + for (const Expr *Arg : Args) + ArgDecls.insert_range(referencedDecls(*Arg)); + if (!LHS.HasSideEffects(Ctx) && readsOnlyUnmentionedLocals(LHS, ArgDecls)) + return true; + const DeclSet LHSDecls = referencedDecls(LHS); + return llvm::all_of(Args, [&](const Expr *Arg) { + return isUnaffectedArgument(*Arg, LHSDecls, Ctx); + }); +} + +static const Stmt *getParentStmt(const Stmt &S, ASTContext &Ctx) { + const DynTypedNodeList Parents = Ctx.getParents(S); + return Parents.size() == 1 ? Parents[0].get<Stmt>() : nullptr; +} + +// 'assign', 'clear' and 'resize' do not yield the container. +static bool isValueDiscarded(const Expr &E, ASTContext &Ctx) { + const Stmt *Child = &E; + const Stmt *Parent = getParentStmt(E, Ctx); + while (isa_and_nonnull<ParenExpr, ExprWithCleanups>(Parent)) { + Child = Parent; + Parent = getParentStmt(*Parent, Ctx); + } + if (!Parent) + return false; + if (const auto *Block = dyn_cast<CompoundStmt>(Parent)) + return Block->body_back() != Child || + !isa_and_nonnull<StmtExpr>(getParentStmt(*Block, Ctx)); + if (const auto *If = dyn_cast<IfStmt>(Parent)) + return Child != If->getCond(); + if (const auto *For = dyn_cast<ForStmt>(Parent)) + return Child != For->getCond(); + if (const auto *While = dyn_cast<WhileStmt>(Parent)) + return Child == While->getBody(); + if (const auto *Do = dyn_cast<DoStmt>(Parent)) + return Child == Do->getBody(); + if (const auto *Range = dyn_cast<CXXForRangeStmt>(Parent)) + return Child == Range->getBody(); + return isa<CaseStmt, DefaultStmt, LabelStmt, AttributedStmt>(Parent); +} + +static std::optional<FixItHint> +createFix(Rewrite Form, const CXXOperatorCallExpr &Op, const Expr &LHS, + ArrayRef<const Expr *> Args, ASTContext &Ctx) { + if (Op.getBeginLoc().isMacroID() || Op.getEndLoc().isMacroID()) + return std::nullopt; + + if (!isValueDiscarded(Op, Ctx) || + !isEvaluationOrderIndependent(LHS, Args, Ctx)) + return std::nullopt; + + const std::optional<std::string> Access = tooling::buildAccess(LHS, Ctx); + if (!Access) + return std::nullopt; + SmallVector<StringRef, 4> ArgTexts; + for (const Expr *Arg : + Form == Rewrite::AssignRange ? Args.drop_front() : Args) { + ArgTexts.push_back(tooling::getText(*Arg, Ctx)); + if (ArgTexts.back().empty()) + return std::nullopt; + } + + const StringRef Method = + Form == Rewrite::AssignRange ? "assign_range" : "assign"; + return FixItHint::CreateReplacement(Op.getSourceRange(), + *Access + Method.str() + "(" + + llvm::join(ArgTexts, ", ") + ")"); +} + +InefficientContainerAssignmentCheck::InefficientContainerAssignmentCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + ContainerClasses(utils::options::parseStringList(Options.get( + "ContainerClasses", "::std::vector;::std::deque;::std::list;" + "::std::forward_list;::std::basic_string"))) {} + +void InefficientContainerAssignmentCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "ContainerClasses", + utils::options::serializeStringList(ContainerClasses)); +} + +void InefficientContainerAssignmentCheck::registerMatchers( + MatchFinder *Finder) { + if (ContainerClasses.empty()) + return; + + const auto Container = cxxRecordDecl(hasAnyName(ContainerClasses)); + const auto Temporary = + cxxConstructExpr(hasDeclaration(cxxConstructorDecl(ofClass(Container)))) + .bind("ctor"); + + // A rewrite might not fit every instantiation of a template. + Finder->addMatcher( + cxxOperatorCallExpr( + unless(isInTemplateInstantiation()), hasOverloadedOperatorName("="), + callee(cxxMethodDecl(ofClass(Container))), + hasArgument(0, expr().bind("lhs")), + hasArgument(1, ignoringImplicit(anyOf( + Temporary, cxxFunctionalCastExpr(has( + ignoringImplicit(Temporary))))))) + .bind("op"), + this); +} + +void InefficientContainerAssignmentCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Op = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("op"); + const auto *LHS = Result.Nodes.getNodeAs<Expr>("lhs"); + const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor"); + ASTContext &Ctx = *Result.Context; + + // A derived class or another specialization would go through a different + // 'operator='. + if (!ASTContext::hasSameUnqualifiedType(LHS->getType(), Ctor->getType())) + return; + const QualType Element = getElementType(Ctor->getType()); + if (Element.isNull()) + return; + + const CXXConstructorDecl *CtorDecl = Ctor->getConstructor(); + SmallVector<const Expr *, 4> Args; + for (const Expr *Arg : Ctor->arguments()) + if (!isa<CXXDefaultArgExpr>(Arg)) + Args.push_back(Arg); + + // 'x = Container();' releases the storage, which 'clear()' would keep. + if (Args.empty() || + (Args.size() >= 2 && passesAllocator(*CtorDecl, Args.size()))) + return; + + const std::optional<Rewrite> Form = + getRewrite(*CtorDecl, Args, *LHS, Element, Ctx); + if (!Form) + return; + + const auto Diag = diag(Op->getOperatorLoc(), + "inefficient assignment from a temporary '%0'; " + "%select{use 'assign'|use 'assign_range'}1 " + "to reuse the existing storage") + << CtorDecl->getParent()->getQualifiedNameAsString() + << static_cast<int>(*Form); + if (std::optional<FixItHint> Fix = createFix(*Form, *Op, *LHS, Args, Ctx)) + Diag << *Fix; +} + +} // namespace clang::tidy::performance diff --git a/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.h b/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.h new file mode 100644 index 00000000000000..fb1a77dbfe4091 --- /dev/null +++ b/clang-tools-extra/clang-tidy/performance/InefficientContainerAssignmentCheck.h @@ -0,0 +1,39 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCONTAINERASSIGNMENTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCONTAINERASSIGNMENTCHECK_H + +#include "../ClangTidyCheck.h" +#include <vector> + +namespace clang::tidy::performance { + +/// Finds assignments from temporary containers that could reuse the storage +/// of the destination. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-container-assignment.html +class InefficientContainerAssignmentCheck : public ClangTidyCheck { +public: + InefficientContainerAssignmentCheck(StringRef Name, + ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + +private: + const std::vector<StringRef> ContainerClasses; +}; + +} // namespace clang::tidy::performance + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_INEFFICIENTCONTAINERASSIGNMENTCHECK_H diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp index 9eee02494be919..6115b9aad0f518 100644 --- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp @@ -14,6 +14,7 @@ #include "ForRangeCopyCheck.h" #include "ImplicitConversionInLoopCheck.h" #include "InefficientAlgorithmCheck.h" +#include "InefficientContainerAssignmentCheck.h" #include "InefficientStringConcatenationCheck.h" #include "InefficientVectorOperationCheck.h" #include "MoveConstArgCheck.h" @@ -50,6 +51,8 @@ class PerformanceModule : public ClangTidyModule { "performance-implicit-conversion-in-loop"); CheckFactories.registerCheck<InefficientAlgorithmCheck>( "performance-inefficient-algorithm"); + CheckFactories.registerCheck<InefficientContainerAssignmentCheck>( + "performance-inefficient-container-assignment"); CheckFactories.registerCheck<InefficientStringConcatenationCheck>( "performance-inefficient-string-concatenation"); CheckFactories.registerCheck<InefficientVectorOperationCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 679ca9ecbb4a39..f33ff006e9f8d2 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -168,6 +168,12 @@ infrastructure are described first, followed by tool-specific sections. Finds calls to `value_or` (and alternative spellings `valueOr`, `ValueOr`) on optional types where the return type is expensive to copy. +- New {doc}`performance-inefficient-container-assignment + <clang-tidy/checks/performance/inefficient-container-assignment>` check. + + Finds assignments of a temporary container to a container of the same type + that could reuse the storage of the destination instead. + - New {doc}`portability-avoid-pragma-comment <clang-tidy/checks/portability/avoid-pragma-comment>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.md b/clang-tools-extra/docs/clang-tidy/checks/list.md index ec20c380131852..4e97ec0b12e892 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.md +++ b/clang-tools-extra/docs/clang-tidy/checks/list.md @@ -363,6 +363,7 @@ readability/* | {doc}`performance-for-range-copy <performance/for-range-copy>` | Yes | | {doc}`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>` | | | {doc}`performance-inefficient-algorithm <performance/inefficient-algorithm>` | Yes | +| {doc}`performance-inefficient-container-assignment <performance/inefficient-container-assignment>` | Yes | | {doc}`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation>` | | | {doc}`performance-inefficient-vector-operation <performance/inefficient-vector-operation>` | Yes | | {doc}`performance-move-const-arg <performance/move-const-arg>` | Yes | diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-container-assignment.md b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-container-assignment.md new file mode 100644 index 00000000000000..0f21885fa31d5e --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/performance/inefficient-container-assignment.md @@ -0,0 +1,39 @@ +```{title} clang-tidy - performance-inefficient-container-assignment +``` + +# performance-inefficient-container-assignment + +Finds assignments of a temporary container to a container of the same type +that could reuse the storage of the destination instead. The temporary +allocates its own buffer, and the move assignment then frees the one that the +destination already owns. + +```cpp +v = std::vector<int>(n, 0); // v.assign(n, 0); +v = std::vector<int>(first, last); // v.assign(first, last); +v = {first, last}; // v.assign(first, last); +v = std::vector<int>(std::from_range, r); // v.assign_range(r); +s = std::string(p, n); // s.assign(p, n); +``` + +The check does not diagnose assignments where the replacement would not +compile or would behave differently: when the elements are not assignable, +when an argument refers into the destination, and when an allocator is +passed. `v = Container()` is not diagnosed either, because it releases the +storage. + +The fix-it is omitted inside macros, when the value of the assignment is +used, and when evaluating the destination before the arguments could change +the result. + +Unlike the assignment, the replacements leave the destination in a valid but +unspecified state if copying an element throws. + +## Options + +```{option} ContainerClasses + +Semicolon-separated list of fully qualified names of container classes with +the interface of the standard sequence containers. Default is +`::std::vector;::std::deque;::std::list;::std::forward_list;::std::basic_string`. +``` diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/deque b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/deque index 9f7393b4673216..17717b9d8a7246 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/deque +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/deque @@ -35,6 +35,7 @@ public: deque() = default; deque(initializer_list<T>) {} + deque(size_t count, const T &value); iterator begin(); const_iterator begin() const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/forward_list b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/forward_list index b2a6521c0d331d..272c7d6793b238 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/forward_list +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/forward_list @@ -34,6 +34,7 @@ public: forward_list() = default; forward_list(initializer_list<T>) {} + forward_list(size_t count, const T &value); iterator begin(); const_iterator begin() const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/list b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/list index a5ea99180869a3..285ebb6f4065da 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/list +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/list @@ -35,6 +35,7 @@ public: list() = default; list(initializer_list<T>) {} + list(size_t count, const T &value); iterator begin(); const_iterator begin() const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string index 766f240c655fbc..7e0d62a8d9df02 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/string @@ -28,6 +28,7 @@ struct basic_string { basic_string(const C *p, size_type count); basic_string(const C *b, const C *e); basic_string(size_t, C); + basic_string(const basic_string &str, size_type pos, size_type n = npos); operator basic_string_view<C, T>() const; ~basic_string(); @@ -44,6 +45,8 @@ struct basic_string { _Type& append(const C *s, size_type n); _Type& assign(const C *s); _Type& assign(const C *s, size_type n); + _Type& assign(size_type count, C ch); + _Type& assign(const basic_string &str, size_type pos, size_type n = npos); int compare(const _Type&) const; int compare(const C* s) const; diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/vector b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/vector index e7d85f9ecce918..2f576f348a142a 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/vector +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/std/vector @@ -6,6 +6,13 @@ namespace std { +#if __cplusplus >= 202302L +struct from_range_t { + explicit from_range_t() = default; +}; +inline constexpr from_range_t from_range{}; +#endif + template <typename T, typename A = allocator<T>> class vector { public: @@ -19,7 +26,13 @@ public: vector(); vector(size_type count); vector(size_type count, const T &value, const A &alloc = A()); + template <typename InputIt, typename = decltype(*InputIt())> + vector(InputIt first, InputIt last, const A &alloc = A()); vector(initializer_list<T>, const A &alloc = A()); +#if __cplusplus >= 202302L + template <typename R> + vector(from_range_t, R &&range, const A &alloc = A()); +#endif vector(const vector &other); vector(vector &&other); ~vector(); @@ -68,6 +81,13 @@ public: void resize(size_type count, const T &value); void assign(size_type count, const T &value); + template <typename InputIt, typename = decltype(*InputIt())> + void assign(InputIt first, InputIt last); + void assign(initializer_list<T>); +#if __cplusplus >= 202302L + template <typename R> + void assign_range(R &&range); +#endif iterator insert(const_iterator pos, const T &value); iterator insert(const_iterator pos, T &&value); diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment-cxx23.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment-cxx23.cpp new file mode 100644 index 00000000000000..685815ac4e97a9 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment-cxx23.cpp @@ -0,0 +1,31 @@ +// RUN: %check_clang_tidy -std=c++23-or-later %s performance-inefficient-container-assignment %t + +#include <vector> + +void fromRange(std::vector<int> &V, const std::vector<int> &Other, + const int (&Array)[3]) { + V = std::vector<int>(std::from_range, Other); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign_range' to reuse the existing storage [performance-inefficient-container-assignment] + // CHECK-FIXES: V.assign_range(Other); + + V = std::vector<int>(std::from_range, Array); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign_range' + // CHECK-FIXES: V.assign_range(Array); +} + +void fromRangeWithAllocator(std::vector<int> &V, const std::vector<int> &Other, + std::allocator<int> Alloc) { + // An explicitly passed allocator has no counterpart in 'assign_range'. + V = std::vector<int>(std::from_range, Other, Alloc); +} + +struct NoCopyAssign { + NoCopyAssign(const NoCopyAssign &); + NoCopyAssign &operator=(const NoCopyAssign &) = delete; +}; + +void notAssignable(std::vector<NoCopyAssign> &V, + const std::vector<NoCopyAssign> &Other) { + // 'assign_range' needs assignable elements. + V = std::vector<NoCopyAssign>(std::from_range, Other); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment.cpp new file mode 100644 index 00000000000000..99d5fa4a26a7f7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/inefficient-container-assignment.cpp @@ -0,0 +1,245 @@ +// RUN: %check_clang_tidy %s performance-inefficient-container-assignment %t + +#include <deque> +#include <forward_list> +#include <list> +#include <string> +#include <utility> +#include <vector> + +using size_type = std::vector<int>::size_type; + +std::vector<int> &getVector(); +size_type getCount(); + +struct Holder { + std::vector<int> Items; + std::vector<int> *Ptr; + size_type Count; +}; + +void fill(std::vector<int> &V, size_type N) { + V = std::vector<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' to reuse the existing storage [performance-inefficient-container-assignment] + // CHECK-FIXES: V.assign(N, 0); + + V = std::vector<int>(N + 1, -1); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(N + 1, -1); +} + +void range(std::vector<int> &V, const int *Begin, const int *End, + const std::vector<int> &Other) { + V = std::vector<int>(Begin, End); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(Begin, End); + + V = std::vector<int>(Other.begin(), Other.end()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(Other.begin(), Other.end()); +} + +void convertingRange(std::vector<std::string> &V, const char *const *Begin, + const char *const *End, char **Argv, int Argc) { + V = std::vector<std::string>(Begin, End); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(Begin, End); + + V = std::vector<std::string>(Argv + 1, Argv + Argc); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(Argv + 1, Argv + Argc); +} + +void bracedRange(std::vector<int> &V, const int *Begin, const int *End, + std::string &S, const char *Chars, size_type N) { + V = {Begin, End}; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(Begin, End); + + S = {Chars, N}; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::basic_string'; use 'assign' + // CHECK-FIXES: S.assign(Chars, N); +} + +void destinations(Holder &H, size_type N) { + H.Items = std::vector<int>(N, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: H.Items.assign(N, 1); + + *H.Ptr = std::vector<int>(N, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: H.Ptr->assign(N, 1); + + getVector() = std::vector<int>(N, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: getVector().assign(N, 2); +} + +struct Member { + std::vector<int> Items; + + void reset(size_type N) { + Items = std::vector<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: Items.assign(N, 0); + } +}; + +void singleStatementBody(std::vector<int> &V, size_type N) { + if (N) + V = std::vector<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(N, 0); +} + +void parenthesized(std::vector<int> &V, size_type N) { + (V = std::vector<int>(N, 0)); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: (V.assign(N, 0)); +} + +void evaluationOrder(std::vector<std::vector<int>> &VS, std::vector<int> *Arr, + size_type I) { + // The assignment evaluates the temporary before the destination, whereas + // 'assign' evaluates the destination first. + VS[I++] = std::vector<int>(I + 1, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: VS[I++] = std::vector<int>(I + 1, 0); + + VS[I] = std::vector<int>(I++, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: VS[I] = std::vector<int>(I++, 0); + + Arr[I] = std::vector<int>(getCount(), 0); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: Arr[I].assign(getCount(), 0); + + VS.back() = std::vector<int>(I, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: VS.back().assign(I, 0); +} + +void aliasing(std::vector<int> &V, Holder &H, size_type N) { + // 'assign' must not be given iterators into the container it replaces. + V = std::vector<int>(V.begin() + 1, V.end()); + V = std::vector<int>(V.end() - N, V.end()); + + // A count is computed before 'assign' runs. + V = std::vector<int>(V.size(), 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: V.assign(V.size(), 0); + + H.Items = std::vector<int>(H.Count, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: H.Items.assign(H.Count, 1); +} + +void valueUsed(std::vector<int> &V, std::vector<int> &W, size_type N) { + W = (V = std::vector<int>(N, 0)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: W = (V = std::vector<int>(N, 0)); +} + +std::vector<int> &returned(std::vector<int> &V, size_type N) { + return V = std::vector<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: return V = std::vector<int>(N, 0); +} + +void otherContainers(std::deque<int> &D, std::list<int> &L, + std::forward_list<int> &F, size_type N) { + D = std::deque<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::deque'; use 'assign' + // CHECK-FIXES: D.assign(N, 0); + + L = std::list<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::list'; use 'assign' + // CHECK-FIXES: L.assign(N, 0); + + F = std::forward_list<int>(N, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::forward_list'; use 'assign' + // CHECK-FIXES: F.assign(N, 0); +} + +void strings(std::string &S, const std::string &Other, const char *Chars, + size_type N) { + S = std::string(N, ' '); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::basic_string'; use 'assign' + // CHECK-FIXES: S.assign(N, ' '); + + S = std::string(Chars, N); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::basic_string'; use 'assign' + // CHECK-FIXES: S.assign(Chars, N); + + S = std::string(Other, 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: inefficient assignment from a temporary 'std::basic_string'; use 'assign' + // CHECK-FIXES: S.assign(Other, 1, 2); + + // A conversion rather than a copy. + S = std::string(Chars); +} + +struct NoCopyAssign { + NoCopyAssign(const NoCopyAssign &); + NoCopyAssign &operator=(const NoCopyAssign &) = delete; +}; + +struct ConstMember { + const int Value; +}; + +struct ExplicitFromInt { + explicit ExplicitFromInt(int); +}; + +void elementRequirements(std::vector<NoCopyAssign> &V, + const NoCopyAssign &Value, const NoCopyAssign *Begin, + const NoCopyAssign *End, std::vector<ConstMember> &C, + const ConstMember &Member, + std::vector<ExplicitFromInt> &E, const int *Ints, + size_type N) { + // 'assign' needs assignable elements, which constructing the temporary does + // not. + V = std::vector<NoCopyAssign>(N, Value); + V = std::vector<NoCopyAssign>(Begin, End); + C = std::vector<ConstMember>(N, Member); + E = std::vector<ExplicitFromInt>(Ints, Ints + N); +} + +void notDiagnosed(std::vector<int> &V, size_type N, + std::allocator<int> Alloc) { + // Default construction releases the storage; 'clear()' would keep it. + V = std::vector<int>(); + V = std::vector<int>{}; + + // No temporary container is involved. + V = {}; + V = {1, 2}; + + // An explicitly passed allocator has no counterpart in 'assign'. + V = std::vector<int>(N, 0, Alloc); + + // Initializations are copy-elided since C++17. + std::vector<int> Init = std::vector<int>(N, 0); +} + +struct Derived : std::vector<int> { + using std::vector<int>::operator=; +}; + +void derived(Derived &D, size_type N) { + D = std::vector<int>(N, 0); +} + +template <typename T> +void dependent(std::vector<T> &V, size_type N) { + V = std::vector<T>(N, T()); +} +void instantiate(std::vector<int> &V) { dependent(V, 3); } + +#define ASSIGN_FILL(Dest, Count) Dest = std::vector<int>(Count, 0) +void macro(std::vector<int> &V) { + ASSIGN_FILL(V, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: inefficient assignment from a temporary 'std::vector'; use 'assign' + // CHECK-FIXES: ASSIGN_FILL(V, 3); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
