-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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.
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
[clang-tidy] Fix bug in modernize-use-emplace #66169
Conversation
emplace_back cannot construct an aggregate with arguments used to initialize the aggregate. Closes llvm#62387 Test plan: Added test case from llvm#62387 which contains code that should not be replaced by the check.
d4379ee
to
d8c40cc
Compare
@llvm/pr-subscribers-clang-tidy Changesemplace_back cannot construct an aggregate with arguments used to initialize the aggregate. Closes #62387Test plan: Added test test from #62387 which contains code that should not be replaced by the check.Full diff: https://github.com/llvm/llvm-project/pull/66169.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index 554abcd900e329c..e4455d6f9c1feec 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -13,6 +13,10 @@ using namespace clang::ast_matchers; namespace clang::tidy::modernize { namespace { +AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) { + return Node.getNumInits() == N; +} + // Identical to hasAnyName, except it does not take template specifiers into // account. This is used to match the functions names as in // DefaultEmplacyFunctions below without caring about the template types of the @@ -207,11 +211,13 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr)); // allow for T{} to be replaced, even if no CTOR is declared - auto HasConstructInitListExpr = has(initListExpr(anyOf( - allOf(has(SoughtConstructExpr), - has(cxxConstructExpr(argumentCountIs(0)))), - has(cxxBindTemporaryExpr(has(SoughtConstructExpr), - has(cxxConstructExpr(argumentCountIs(0)))))))); + auto HasConstructInitListExpr = + has(initListExpr(anyOf(initCountIs(0), initCountIs(1)), + anyOf(allOf(has(SoughtConstructExpr), + has(cxxConstructExpr(argumentCountIs(0)))), + has(cxxBindTemporaryExpr( + has(SoughtConstructExpr), + has(cxxConstructExpr(argumentCountIs(0)))))))); auto HasBracedInitListExpr = anyOf(has(cxxBindTemporaryExpr(HasConstructInitListExpr)), HasConstructInitListExpr); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 19c977977f9044c..694beeb98a54e36 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -247,6 +247,10 @@ Changes in existing checks <clang-tidy/checks/modernize/loop-convert>` to support for-loops with iterators initialized by free functions like ``begin``, ``end``, or ``size``. +- Improved :doc:`modernize-use-emplace + <clang-tidy/checks/modernize/use-emplace>` to not replace aggregates that + ``emplace_back`` cannot construct with aggregate initialization. + - Improved :doc:`modernize-use-equals-delete <clang-tidy/checks/modernize/use-equals-delete>` check to ignore false-positives when special member function is actually used or implicit. 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 fead2b6151d0218..74edf0760bb324d 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 @@ -1183,6 +1183,11 @@ struct NonTrivialWithVector { std::vector<int> it; }; +struct NonTrivialWithIntAndVector { + int x; + std::vector<int> it; +}; + struct NonTrivialWithCtor { NonTrivialWithCtor(); NonTrivialWithCtor(std::vector<int> const&); @@ -1332,6 +1337,17 @@ void testBracedInitTemporaries() { v3.push_back(NonTrivialWithCtor{{}}); v3.push_back({{0}}); v3.push_back({{}}); + + std::vector<NonTrivialWithIntAndVector> v4; + + // These should not be noticed or fixed; after the correction, the code won't + // compile. + v4.push_back(NonTrivialWithIntAndVector{1, {}}); + // CHECK-FIXES: v4.push_back(NonTrivialWithIntAndVector{1, {}}); + v4.push_back(NonTrivialWithIntAndVector{}); + // CHECK-FIXES: v4.push_back(NonTrivialWithIntAndVector{}); + v4.push_back({}); + // CHECK-FIXES: v4.push_back({}); } void testWithPointerTypes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, check comments, and verify if check shouldn't be made more strict.
clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp
Outdated
Show resolved
Hide resolved
clang-format need to be run on this change. |
Manually formating code via clang-format after previous commit merge.
emplace_back cannot construct an aggregate with arguments used to initialize the aggregate.
Closes #62387
Test plan: Added test test from #62387 which contains code that should not be replaced by the check.