[clang-tools-extra] [clang-tidy] Add support for aggregate types within modernize-use-emplace (PR #131969)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 18 20:51:23 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
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
More information about the cfe-commits
mailing list