Page MenuHomePhabricator

[clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce
ClosedPublic

Authored by kuhar on May 2 2017, 3:57 PM.

Details

Summary

This patch fixes PR32896.

The problem was that modernize-use-emplace incorrectly removed changed push_back into emplace_back, removing explicit constructor call with initializer list parameter, resulting in compiler error after applying fixits.
modernize-use-emplace used to check if matched constructor had InitListExpr, but didn't check against CXXStdInitializerListExpr.

Eg.

std::vector<std::vector<int>> v;
  v.push_back(std::vector<int>({1})); // --> v.emplace_back({1});

Diff Detail

Event Timeline

kuhar created this revision.May 2 2017, 3:57 PM
alexfh edited edge metadata.May 3 2017, 7:00 AM

Thank you! One comment inline.

clang-tidy/modernize/UseEmplaceCheck.cpp
23

This should be a node matcher rather than a narrowing matcher, and it should be placed to ASTMatchers.h, if there is no such matcher already.

kuhar added inline comments.May 3 2017, 7:18 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
23

I wanted to backport this fix to 4.0.1 release after applying it in trunk.
I thought that adding a new ASTMatcher would introduce an API change in clang, and I'm not sure if it's much welcome.

Maybe it would be better to introduce a new matcher in ASTMatchers.h in trunk and make it an internal narrowing matcher in the backported patch? What would be best?

Prazek added inline comments.May 3 2017, 9:14 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
23

Why adding new matcher to 4.0.1 release is bad? Will it break anything?

kuhar added inline comments.May 3 2017, 9:20 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
23

The example that I originally came up with was updating clang-tidy to 4.0.1 while keeping clang libs 4.0.0, but since we just include all the matchers I think it should work fine.

Here's a patch with the new matcher: https://reviews.llvm.org/D32810

aaron.ballman added inline comments.May 3 2017, 11:13 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
23

Why adding new matcher to 4.0.1 release is bad? Will it break anything?

Because it doesn't meet the criteria for doing a point release, generally speaking.

I think a better approach is to leave this matcher as part of this patch, get the patch submitted, then Tom can cherrypick the commit for 4.0.1. You can then submit a second patch to hoist this matcher out to ASTMatchers.h and use it within this check. (Basically do it in two steps.)

24

This looks odd to me. Can you do:

const internal::VariadicDynCastAllOfMatcher<Expr, CXXStdInitializerListExpr> cxxStdInitializerListExpr;

I'm not certain whether this works or not, but we don't have matchers that just return true.

kuhar updated this revision to Diff 97704.May 3 2017, 12:17 PM

I switched from a narrowing matcher to a "normal" node matcher.

kuhar marked 2 inline comments as done.May 3 2017, 12:18 PM
This revision is now accepted and ready to land.May 3 2017, 12:22 PM
Prazek accepted this revision.May 5 2017, 9:58 AM

LGTM

This revision was automatically updated to reflect the committed changes.