Page MenuHomePhabricator

[clang-tidy] Fixes to modernize-use-emplace
ClosedPublic

Authored by Prazek on Jul 10 2016, 10:41 PM.

Details

Summary

Not everything is valid (as you see in FIXME), but it should works for 99.8% cases.

I gonna fix it in next week, but I would like to land this bug fixes right now.

This is mainly fixes of the review that @sbenza gave me last time.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 63461.Jul 10 2016, 10:41 PM
Prazek retitled this revision from to clang-tidy] Fixes to modernize-use-emplace.
Prazek updated this object.
Prazek added reviewers: sbenza, alexfh, aaron.ballman.
Prazek set the repository for this revision to rL LLVM.
Prazek added a project: Restricted Project.
Prazek added subscribers: cfe-commits, hokein, sbarzowski and 3 others.

BTW I've made clang-extra project on phabricator as you can see. Please use it, it will be much easier to filter reviews.

aaron.ballman added inline comments.Jul 11 2016, 11:12 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
21

Should be Names instead.

21

Is this needed? We have the hasAnyName() matcher already.

31–33

These should start with a capital as well (coding standard information pertaining to names can be found at http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

32–34

These should be using ::std:: instead of std:: just in case people do awful things like embed namespace std inside of their own namespaces.

clang-tidy/modernize/UseEmplaceCheck.h
36

Should be Containers and Smart (capital letters).

Prazek added inline comments.Jul 11 2016, 2:44 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
21

The problem is that this matcher takes variadic number of strings. Here I have a vector of strings. sbenza did similar thing in performance-fast-string-find.

In the future I would like to add overload of the matcher that would take a vector of strings (it is is possible),
or at least move this function somewhere.

Another idea would be to return matcher instead of Optional<Matcher> - if list is empty then return unless(anything()) - there should be nothing() matcher :P

31–33

thanks, I always though that only local variables have to start with capital.

Prazek added inline comments.Jul 12 2016, 10:38 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

There is a bug here that I forgot about. Should be InnerCtorCall->getStartLoc()

Prazek updated this revision to Diff 64238.Jul 16 2016, 5:23 PM
Prazek removed rL LLVM as the repository for this revision.
Prazek marked 6 inline comments as done.
Prazek set the repository for this revision to rL LLVM.
aaron.ballman added inline comments.Jul 19 2016, 6:20 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
21

Looking at VariadicFunction, it appears that it already works if you pass an ArrayRef<T> to the operator()() overload. See ASTMatchersInternal.h:81. So I still think the matcher can be used directly, just with changing the type of the object passed to the functor.

alexfh requested changes to this revision.Jul 19 2016, 3:55 PM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseEmplaceCheck.cpp
31

A std::string is not needed here, please change to const char [].

clang-tidy/modernize/UseEmplaceCheck.h
36

Declarations with multiple variables can be misleading. Please split this into two separate declarations.

docs/clang-tidy/checks/modernize-use-emplace.rst
6–17

IMO, this sentence fails to convey the most important parts of the information. Consider changing to something like this:

The check flags insertions to an STL-style container done by calling the push_back method with an explicitly-constructed temporary of the container element type. In this case, the corresponding emplace_back method results in less verbose and potentially more efficient code.

Maybe also explain, why the check only looks for push_back, and not insert, push_front and push.

9

s/; separated/semicolon-separated/

This revision now requires changes to proceed.Jul 19 2016, 3:55 PM
Prazek marked 5 inline comments as done.Jul 19 2016, 10:27 PM

Aaron, Alex thanks for the review. After running it on llvm I also have found the private constructor bug so I also fixed it.

clang-tidy/modernize/UseEmplaceCheck.cpp
21

That's cool! I convert the vector to SmallVector, because ArrayRef doesn't have ctor taking iterators.

I will later also change the code in FasterStringFind that also do this kind of things.

Prazek updated this revision to Diff 64640.Jul 19 2016, 10:31 PM
Prazek edited edge metadata.
Prazek marked an inline comment as done.
aaron.ballman added inline comments.Jul 21 2016, 8:37 AM
clang-tidy/modernize/UseEmplaceCheck.h
36–37

Why not use a SmallVector for these instead of a std::vector? Then you don't need to typecast in registerMatchers().

If it's because of parseStringList(), I think that you can work around it's tight coupling using llvm::iterator_range(parseStringList()) to perform the initialization in the ctor initializer list.

Prazek added inline comments.Jul 21 2016, 3:38 PM
clang-tidy/modernize/UseEmplaceCheck.h
36–37

Unfortunatelly it doesn't want to work. I tried llvm::iterator_range<std::string*> and llvm::iterator_range<SmallVector<std::string, 5>::iterator> and it doesn't compile.

Prazek retitled this revision from clang-tidy] Fixes to modernize-use-emplace to [clang-tidy] Fixes to modernize-use-emplace.Jul 21 2016, 5:45 PM
Prazek edited edge metadata.
alexfh requested changes to this revision.Jul 21 2016, 5:55 PM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/modernize/UseEmplaceCheck.cpp
45–46

What's the reason for explicitly creating a SmallVector? VariadicFunction::operator() takes an ArrayRef, which std::vector<> should be implicitly convertible to. Am I missing something?

docs/clang-tidy/checks/modernize-use-emplace.rst
12

s/chaning/changing/

12

How about the insert method of std::vector<>?

This revision now requires changes to proceed.Jul 21 2016, 5:55 PM
Prazek added inline comments.Jul 21 2016, 6:07 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
45–46

It is not. I guess it is because it is ArrayRef<StringRef> and I have std::vector<std::string>

docs/clang-tidy/checks/modernize-use-emplace.rst
12

insert -> emplace should be fine for the vector, but this patch is trying to mainly fix the bugs (and I want it to land in clang-3.9).

I will add `insert near the push_front`

alexfh added inline comments.Jul 21 2016, 6:21 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
45–46

Fair enough.

115

There is a bug here that I forgot about. Should be InnerCtorCall->getStartLoc()

Is this still relevant?

docs/clang-tidy/checks/modernize-use-emplace.rst
12

SG

Prazek added inline comments.Jul 21 2016, 7:04 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

The thing is, that I have no idea. So I was expecting this to happen when I will run on llvm code base, but it seems that it works I don't know why.

So firstly, I marked wrong line, because I think the problem might be in line 94 (Call->getArg(0)->getExprLoc() should be ->getLocStart())

So what I was expecting that if The Call->getArg(0) will be a memberExpr (call to member function) then getExprLoc will point to dot instead of the beggining of argument, so
we would remove a object name like

v.push_back(obj.member());
into
v.emplace_back(member());

But I have no idea why I can't find it, and right now I don't know if it is a real bug or not, but it should be if I got a bug here
https://reviews.llvm.org/D21642

Do you have any thoughts/test case that would break it?

There is one bug left:

In the ClangIncludeFixer.cpp:169 there is push back that looks like this

Symbols.push_back(find_all_symbols::SymbolInfo(

Split.first.trim(),
find_all_symbols::SymbolInfo::SymbolKind::Unknown,
CommaSplits[I].trim(), 1, {}, /*NumOccurrences=*/E - I));

There is initializer list inside that should not allow this to be matched (because I check for initializer list as argument)

Unfortunatelly clang AST doesn't mention any initializer list.

http://wklejto.pl/258691

As you can see the "{}" looks like this in AST

|-MaterializeTemporaryExpr 0x7f759d803900 <col:39, col:40> 'const std::vector<Context>':'const class std::vector<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> >, class std::allocator<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> > > >' lvalue
|   |   |               | `-CXXBindTemporaryExpr 0x7f759d8038e0 <col:39, col:40> 'const std::vector<Context>':'const class std::vector<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> >, class std::allocator<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> > > >' (CXXTemporary 0x7f759d8038d8)
|   |   |               |   `-CXXConstructExpr 0x7f759d8038a8 <col:39, col:40> 'const std::vector<Context>':'const class std::vector<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> >, class std::allocator<struct std::pair<enum clang::find_all_symbols::SymbolInfo::ContextType, class std::basic_string<char> > > >' 'void (void)'
|   |   |

Good news are that this is the only 1 case from all push_backs that it found on LLVM+Clang+extra. This patch fixed the ExprWithCleanups by using ignoreImplicit, so I clang-tidy have many new push_backs.

Prazek added a subscriber: klimek.Jul 21 2016, 7:12 PM

Can you look at my previous comment? I nedd an expert for AST.

Prazek updated this revision to Diff 65023.Jul 21 2016, 8:01 PM
Prazek edited edge metadata.
Prazek removed rL LLVM as the repository for this revision.
Prazek marked 4 inline comments as done.
Prazek edited edge metadata.
Prazek set the repository for this revision to rL LLVM.
Prazek marked 6 inline comments as done.

There is one bug left:

In the ClangIncludeFixer.cpp:169 there is push back that looks like this

Symbols.push_back(find_all_symbols::SymbolInfo(

Split.first.trim(),
find_all_symbols::SymbolInfo::SymbolKind::Unknown,
CommaSplits[I].trim(), 1, {}, /*NumOccurrences=*/E - I));

There is initializer list inside that should not allow this to be matched (because I check for initializer list as argument)

Unfortunatelly clang AST doesn't mention any initializer list.

I guess, CXXConstructExpr::isListInitialization() is what you need to check here.

Prazek updated this revision to Diff 65243.Jul 23 2016, 11:00 AM
Prazek removed rL LLVM as the repository for this revision.

Ok works right now. I don't know why but I could not reproduce the error in test file, but I manged to fix it.

alexfh accepted this revision.Jul 25 2016, 9:58 AM
alexfh edited edge metadata.

LG

clang-tidy/modernize/UseEmplaceCheck.cpp
115

This doesn't seem to be an issue, since expression v.push_back(obj.member()) won't trigger this check: it expects that the argument of the push_back call is a cxxConstructExpr or a cxxFunctionalCastExpr.

This revision is now accepted and ready to land.Jul 25 2016, 9:58 AM
Prazek added inline comments.Jul 26 2016, 11:58 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

what about the implicit conversion? What if obj.member would return object that is different from the one that v stores, but it is convertible to it?

aaron.ballman added inline comments.Jul 28 2016, 7:14 AM
clang-tidy/modernize/UseEmplaceCheck.h
36–37

What about llvm::make_range()?

alexfh added inline comments.Aug 1 2016, 1:14 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

Sounds almost like a recipe for a test case ;) Have you tried to construct it this way?

Prazek added inline comments.Aug 1 2016, 1:59 PM
clang-tidy/modernize/UseEmplaceCheck.h
36–37

llvm::make_range do pretty much the same thing. The patch is in upstream, so feel free to try it, but either I am doing something wrong, or just SmallVector/ArrayRef have this limitations.

Prazek added inline comments.Aug 1 2016, 2:01 PM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

Yes, I think I emulated this case in line 190. I tried to hit this problem but could not find the test case.

Prazek closed this revision.Aug 1 2016, 2:01 PM
alexfh added a comment.Aug 2 2016, 2:49 AM

Please add revision number (this can be automated, if include differential revision URL in your commit message as described in http://llvm.org/docs/Phabricator.html#committing-a-change).

alexfh added inline comments.Aug 2 2016, 3:30 AM
clang-tidy/modernize/UseEmplaceCheck.cpp
115

if The Call->getArg(0) will be a memberExpr (call to member function)

what about the implicit conversion?

I think, I know what happens in this case. When an implicit conversion happens in v.push_back(obj.member()), then Call->getArg(0) is an implicit CXXConstructExpr, not a MemberExpr (MemberExpr will be its indirect child). The case should be handled correctly.

Please add revision number (this can be automated, if include differential revision URL in your commit message as described in http://llvm.org/docs/Phabricator.html#committing-a-change).

I normally use arc, but I was working in half from my laptop and in half from workstation, and I am not sure if arc would handle this.

I added the url in the patch, but I didn't know that "Differential Revision:" is required. I will keep it in mind next time.

Prazek marked 5 inline comments as done.Aug 2 2016, 11:13 AM
Prazek added inline comments.
clang-tidy/modernize/UseEmplaceCheck.cpp
115

That might be it. I remember that in some early version I was biding to MemberExpr somewhere, which might cause it.