llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: David Rivera (RiverDave) <details> <summary>Changes</summary> Aims to address: #<!-- -->115841 C++20 introduced in-place initialization for aggregate types within `emplace_back` calls. I ensured to match any braced init expressions with temporals (and applied similar rules as they were used in constructor expressions) when called with c++20. I'd love to get some feedback related to my test coverage and more possible edge cases I might've missed. --- Full diff: https://github.com/llvm/llvm-project/pull/131969.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp (+49-21) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-cxx20.cpp (+86) - (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp (+1-1) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index 430455a38f395..8c42b3a8a6e3f 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -98,8 +98,8 @@ auto hasWantedType(llvm::ArrayRef<StringRef> TypeNames) { // Matches member call expressions of the named method on the listed container // types. -auto cxxMemberCallExprOnContainer( - StringRef MethodName, llvm::ArrayRef<StringRef> ContainerNames) { +auto cxxMemberCallExprOnContainer(StringRef MethodName, + llvm::ArrayRef<StringRef> ContainerNames) { return cxxMemberCallExpr( hasDeclaration(functionDecl(hasName(MethodName))), on(hasTypeOrPointeeType(hasWantedType(ContainerNames)))); @@ -174,19 +174,19 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { // passed pointer because smart pointer won't be constructed // (and destructed) as in push_back case. auto IsCtorOfSmartPtr = - hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(SmartPointers)))); + cxxConstructorDecl(ofClass(hasAnyName(SmartPointers))); // Bitfields binds only to consts and emplace_back take it by universal ref. - auto BitFieldAsArgument = hasAnyArgument( - ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField()))))); + auto BitFieldAsArgument = + ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField())))); // Initializer list can't be passed to universal reference. - auto InitializerListAsArgument = hasAnyArgument( + auto InitializerListAsArgument = ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()), - unless(cxxTemporaryObjectExpr())))); + unless(cxxTemporaryObjectExpr()))); // We could have leak of resource. - auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr())); + auto NewExprAsArgument = ignoringImplicit(cxxNewExpr()); // We would call another constructor. auto ConstructingDerived = hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase))); @@ -202,19 +202,36 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { // overloaded functions and template names. auto SoughtConstructExpr = cxxConstructExpr( - unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument, - InitializerListAsArgument, NewExprAsArgument, - ConstructingDerived, IsPrivateOrProtectedCtor))) + unless(anyOf(hasDeclaration(IsCtorOfSmartPtr), HasInitList, + hasAnyArgument(BitFieldAsArgument), + hasAnyArgument(InitializerListAsArgument), + hasAnyArgument(NewExprAsArgument), ConstructingDerived, + IsPrivateOrProtectedCtor))) .bind("ctor"); - auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr)); + + auto IsPrimitiveType = hasType(builtinType()); + + auto AggregateInitExpr = + getLangOpts().CPlusPlus20 + ? initListExpr(unless(anyOf(HasInitList, has(IsCtorOfSmartPtr), + has(BitFieldAsArgument), + has(InitializerListAsArgument), + has(NewExprAsArgument), IsPrimitiveType))) + .bind("agg_init") + : unless(anything()); + + auto HasConstructExpr = + has(ignoringImplicit(anyOf(SoughtConstructExpr, AggregateInitExpr))); // allow for T{} to be replaced, even if no CTOR is declared auto HasConstructInitListExpr = has(initListExpr( - initCountLeq(1), anyOf(allOf(has(SoughtConstructExpr), - has(cxxConstructExpr(argumentCountIs(0)))), - has(cxxBindTemporaryExpr( - has(SoughtConstructExpr), - has(cxxConstructExpr(argumentCountIs(0)))))))); + initCountLeq(1), + anyOf(allOf(has(SoughtConstructExpr), + has(cxxConstructExpr(argumentCountIs(0)))), + has(cxxBindTemporaryExpr(has(SoughtConstructExpr), + has(cxxConstructExpr(argumentCountIs(0))) + + ))))); auto HasBracedInitListExpr = anyOf(has(cxxBindTemporaryExpr(HasConstructInitListExpr)), HasConstructInitListExpr); @@ -314,6 +331,7 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { const auto *EmplacyCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call"); const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor"); + const auto *AggInitCall = Result.Nodes.getNodeAs<InitListExpr>("agg_init"); const auto *MakeCall = Result.Nodes.getNodeAs<CallExpr>("make"); const auto *TemporaryExpr = Result.Nodes.getNodeAs<MaterializeTemporaryExpr>("temporary_expr"); @@ -332,12 +350,19 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { }(); assert(Call && "No call matched"); - assert((CtorCall || MakeCall) && "No push_back parameter matched"); + assert((CtorCall || MakeCall || AggInitCall) && + "No push_back parameter matched"); if (IgnoreImplicitConstructors && CtorCall && CtorCall->getNumArgs() >= 1 && CtorCall->getArg(0)->getSourceRange() == CtorCall->getSourceRange()) return; + if (IgnoreImplicitConstructors && AggInitCall && + AggInitCall->getNumInits() >= 1 && + AggInitCall->getInit(0)->getSourceRange() == + AggInitCall->getSourceRange()) + return; + const auto FunctionNameSourceRange = CharSourceRange::getCharRange( Call->getExprLoc(), Call->getArg(0)->getExprLoc()); @@ -345,6 +370,7 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { EmplacyCall ? diag(TemporaryExpr ? TemporaryExpr->getBeginLoc() : CtorCall ? CtorCall->getBeginLoc() + : AggInitCall ? AggInitCall->getBeginLoc() : MakeCall->getBeginLoc(), "unnecessary temporary object created while calling %0") : diag(Call->getExprLoc(), "use emplace%select{|_back|_front}0 " @@ -376,9 +402,10 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { } const SourceRange CallParensRange = - MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(), - MakeCall->getRParenLoc()) - : CtorCall->getParenOrBraceRange(); + MakeCall ? SourceRange(MakeCall->getCallee()->getEndLoc(), + MakeCall->getRParenLoc()) + : CtorCall ? CtorCall->getParenOrBraceRange() + : AggInitCall->getSourceRange(); // Finish if there is no explicit constructor call. if (CallParensRange.getBegin().isInvalid()) @@ -387,6 +414,7 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) { // FIXME: Will there ever be a CtorCall, if there is no TemporaryExpr? const SourceLocation ExprBegin = TemporaryExpr ? TemporaryExpr->getExprLoc() : CtorCall ? CtorCall->getExprLoc() + : AggInitCall ? AggInitCall->getExprLoc() : MakeCall->getExprLoc(); // Range for constructor name and opening brace. diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 72aa05eb4dcd1..125d45ca54c34 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -171,6 +171,10 @@ Changes in existing checks ``constexpr`` and ``static``` values on member initialization and by detecting explicit casting of built-in types within member list initialization. +- Improved :doc:`modernize-use-emplace + <clang-tidy/checks/modernize/use-emplace>` check by adding support for C++20 + aggregate initialization. + - Improved :doc:`modernize-use-ranges <clang-tidy/checks/modernize/use-ranges>` check by updating suppress warnings logic for ``nullptr`` in ``std::find``. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-cxx20.cpp new file mode 100644 index 0000000000000..5b0aa76dc8ab8 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace-cxx20.cpp @@ -0,0 +1,86 @@ +// RUN: %check_clang_tidy %s -std=c++20 modernize-use-emplace %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: {modernize-use-emplace.ContainersWithPushBack: \ +// RUN: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector', \ +// RUN: modernize-use-emplace.TupleTypes: \ +// RUN: '::std::pair; std::tuple; ::test::Single', \ +// RUN: modernize-use-emplace.TupleMakeFunctions: \ +// RUN: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}}" + +namespace std { +template <typename E> +class initializer_list { +public: + const E *a, *b; + initializer_list() noexcept {} +}; + +template <typename T> +class vector { +public: + using value_type = T; + + class iterator {}; + class const_iterator {}; + const_iterator begin() { return const_iterator{}; } + + vector() = default; + vector(initializer_list<T>) {} + + void push_back(const T &) {} + void push_back(T &&) {} + + template <typename... Args> + void emplace_back(Args &&... args){}; + template <typename... Args> + iterator emplace(const_iterator pos, Args &&...args); + ~vector(); +}; + +} // namespace std + +struct InnerType { + InnerType() {} + InnerType(char const*) {} +}; + +//Not aggregate but we should still be able to directly initialize it with emplace_back +struct NonTrivialNoCtor { + InnerType it; +}; + +struct Aggregate { + int a; + int b; +}; + +void testCXX20Cases() { + std::vector<Aggregate> v1; + + v1.push_back(Aggregate{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v1.emplace_back(1, 2); + + v1.push_back({1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v1.emplace_back(1, 2); + + std::vector<NonTrivialNoCtor> v2; + + v2.push_back(NonTrivialNoCtor{""}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v2.emplace_back(""); + + v2.push_back({""}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v2.emplace_back(""); + + v2.push_back(NonTrivialNoCtor{InnerType{""}}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v2.emplace_back(InnerType{""}); + + v2.push_back({InnerType{""}}); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back + // CHECK-FIXES: v2.emplace_back(InnerType{""}); + +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp index e6562cd18dbab..f2390f38c3166 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \ +// RUN: %check_clang_tidy %s -std=c++17 modernize-use-emplace %t -- \ // RUN: -config="{CheckOptions: \ // RUN: {modernize-use-emplace.ContainersWithPushBack: \ // RUN: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector', \ `````````` </details> https://github.com/llvm/llvm-project/pull/131969 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
