Skip to content

[clang-tidy][C++20] Add support for Initialization Forwarding in structs and Nested Objects within modernize-use-emplace #131969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[clang-tidy][C++20] Add support for Initialization Forwarding in structs and Nested Objects within modernize-use-emplace #131969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 86 additions & 17 deletions clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))));
Expand Down Expand Up @@ -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)));
Expand All @@ -202,11 +202,26 @@ 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(
Expand Down Expand Up @@ -305,6 +320,34 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
this);
}

static const CXXConstructExpr *unwrapToConstructorExpr(const Expr *E) {
while (E) {
if (const auto *ConstructorExpr = llvm::dyn_cast<CXXConstructExpr>(E)) {
return ConstructorExpr;
}

if (const auto *BindTemp = llvm::dyn_cast<CXXBindTemporaryExpr>(E)) {
E = BindTemp->getSubExpr();
continue;
}

if (const auto *MaterialTemp =
llvm::dyn_cast<MaterializeTemporaryExpr>(E)) {
E = MaterialTemp->getSubExpr();
continue;
}

if (const auto *Cast = llvm::dyn_cast<ImplicitCastExpr>(E)) {
E = Cast->getSubExpr();
continue;
}

break;
}

return nullptr; // No relevant sub-expression found
}

void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
const auto *PushBackCall =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_back_call");
Expand All @@ -313,7 +356,8 @@ void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
Result.Nodes.getNodeAs<CXXMemberCallExpr>("push_front_call");
const auto *EmplacyCall =
Result.Nodes.getNodeAs<CXXMemberCallExpr>("emplacy_call");
const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctor");
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");
Expand All @@ -332,19 +376,42 @@ 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;

if (getLangOpts().LangStd >= LangStandard::lang_cxx20 && AggInitCall) {
Copy link
Contributor

@5chmidti 5chmidti Jul 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getLangOpts().CPlusPlus20 && ...

for (const auto *Init : AggInitCall->inits()) {
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use real type instead of auto

if (const auto *InnerConstructorExpr = unwrapToConstructorExpr(Init)) {
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use Expr::IgnoreImplicit instead of unwrapToConstructorExpr. Looking at its code, it must do the same.

Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use real type instead of auto

// consume all args if it's an empty constructor call so that we can ->
// T{} -> emplace_back()
if (InnerConstructorExpr && InnerConstructorExpr->getNumArgs() == 0) {
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (InnerConstructorExpr && InnerConstructorExpr->getNumArgs() == 0) {
if (InnerConstructorExpr->getNumArgs() == 0) {

No need to check that InnerConstructorExpr is not null here because it's done in previous if


CtorCall = InnerConstructorExpr;
AggInitCall = nullptr;
break;
}
}
}
}

const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
Call->getExprLoc(), Call->getArg(0)->getExprLoc());

auto Diag =
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 "
Expand Down Expand Up @@ -376,9 +443,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())
Expand All @@ -387,6 +455,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.
Expand Down
4 changes: 4 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-designated-initializers>` check by avoiding
diagnosing designated initializers for ``std::array`` initializations.

- Improved :doc:`modernize-use-emplace
<clang-tidy/checks/modernize/use-emplace>` check by adding support for C++20
Argument forwarding inside ``struct`` type objects.

- Improved :doc:`modernize-use-ranges
<clang-tidy/checks/modernize/use-ranges>` check by updating suppress
warnings logic for ``nullptr`` in ``std::find``.
Expand Down
46 changes: 39 additions & 7 deletions clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
// RUN: %check_clang_tidy %s -std=c++11 modernize-use-emplace %t -- \
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RUN: %check_clang_tidy %s -std=c++11 modernize-use-emplace %t -- \
// RUN: %check_clang_tidy %s -std=c++11,c++14,c++17 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'}}"
// RUN: %check_clang_tidy -std=c++20 %s modernize-use-emplace -check-suffixes=,CPP20 %t -- \
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// RUN: %check_clang_tidy -std=c++20 %s modernize-use-emplace -check-suffixes=,CPP20 %t -- \
// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-emplace -check-suffixes=,CPP20 %t -- \

// RUN: -config="{CheckOptions: \
// RUN: {modernize-use-emplace.ContainersWithPushBack: \
// RUN: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector', \
Expand Down Expand Up @@ -1237,10 +1245,6 @@ void testBracedInitTemporaries() {

// These should not be noticed or fixed; after the correction, the code won't
// compile.
v1.push_back(NonTrivialNoCtor{""});
v1.push_back({""});
v1.push_back(NonTrivialNoCtor{InnerType{""}});
v1.push_back({InnerType{""}});
v1.emplace_back(NonTrivialNoCtor{""});

std::vector<NonTrivialWithVector> v2;
Expand Down Expand Up @@ -1288,8 +1292,6 @@ void testBracedInitTemporaries() {
// compile.
v2.push_back(NonTrivialWithVector{{0}});
v2.push_back({{0}});
v2.push_back(NonTrivialWithVector{std::vector<int>{0}});
v2.push_back({std::vector<int>{0}});
v2.emplace_back(NonTrivialWithVector{std::vector<int>{0}});

std::vector<NonTrivialWithCtor> v3;
Expand Down Expand Up @@ -1434,3 +1436,33 @@ void testWithPointerTypes() {
// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: unnecessary temporary object created while calling emplace
// CHECK-FIXES: sp->emplace();
}

namespace GH1225740 {
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: After N years this could be useless, I'd suggest removing it

Copy link
Contributor

@5chmidti 5chmidti Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have added markers like these. Though I don't have a strong opinion on whether we need them since the git log would link to the PR and therefore resolved issues.

Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, older markers (especially from pre-GitHub era) seems like white noise to me. Usually you can get from the context what the author wanted to say.

This strikes me as code with no version control, where people would put date and description directly in source code🙃


void CXX20testBracedInitTemporaries(){
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void CXX20testBracedInitTemporaries(){
void CXX20testBracedInitTemporaries() {


Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add tests with multiple values in initListExpr(). AFAIK we only have tests with 0 or 1 values.

std::vector<NonTrivialNoCtor> v1;

v1.push_back(NonTrivialNoCtor{""});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v1.emplace_back("");
v1.push_back({""});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v1.emplace_back("");
v1.push_back(NonTrivialNoCtor{InnerType{""}});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v1.emplace_back(InnerType{""});
v1.push_back({InnerType{""}});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v1.emplace_back(InnerType{""});

std::vector<NonTrivialWithVector> v2;

v2.push_back(NonTrivialWithVector{std::vector<int>{0}});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v2.emplace_back(std::vector<int>{0});
v2.push_back({std::vector<int>{0}});
// CHECK-MESSAGES-CPP20: :[[@LINE-1]]:6: warning: use emplace_back instead of push_back
// CHECK-FIXES-CPP20: v2.emplace_back(std::vector<int>{0});
}
Copy link
Contributor

@vbvictor vbvictor Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

}
Loading