modernize-use-emplace only recommends going from a push_back to an
emplace_back, but does not provide a recommendation when emplace_back is
improperly used. This adds the functionality of warning the user when
an unecessary temporary is created while calling emplace_back or other "emplacy"
functions from the STL containers.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
As you can see, this will fail some tests as there is one corner case which is tricky to handle:
std::vector<std::tuple<std::tuple<int, int>>> vec; // This is valid and should not be changed. vec.emplace_back(std::make_tuple(13, 31));
Does anyone have a suggestion for a good way to handle this?
Ideally, it would be nice to just check that the type of the temporary matches the type of the value_type, but I've tried that with a simple ==, and it then becomes a problem with strings, where for example
std::vector<std::pair<std::string, std::string>> vec; vec.emplace_back(std::make_pair("foo", "bar"));
would not be changed as it would list the pair as a std::pair<const char*, const char*>, and not match with std::pair<std::string, std::string>.
Is there some way that I couldn't find to check if two clang::Types are "compatible"?
Please mark fixed suggestions as done.
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp | ||
---|---|---|
248 | Please don't use auto unless type is spelled explicitly in same statement or iterator. |
Thank you for the feedback, I appreciate it! Let me know if there is any other I missed, AFAIK all auto uses introduced by me should be good now.
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp | ||
---|---|---|
29 | I'm not sure, but probably braces could be elided in for and if else. | |
46 | I'm not sure, but probably braces could be elided. | |
57 | Please elide braces. | |
69 | Please elide braces. | |
268 | It's not yours, but please fix it, since code was affected by your change. |
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp | ||
---|---|---|
28 | Can you add a comment explaining what this loops tries to do? Ideally, with a short example like some_func<int> --> ::some_func | |
29 | -1 for removing braces in multi-statement loops | |
46 | -1 | |
294 | Can you pull this ternary expression into a variable so that it does not have to repeated when the diagnostic is emitted? |
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp | ||
---|---|---|
294 | This is a bit tricky, since MakeCall is a CallExpr and CtorCall is a CXXConstructExpr, and they have no common base class. Even if I pull it out it'll be a lot of repetition. I think there may be some elegant way to join the previous removal hints with these, but haven't had time to think about it. I'll probably post another update by the end of this week. |
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp | ||
---|---|---|
44 | nit: StringRef( seems unnecessary on the rhs of = |
Update!
- Rebased.
- Fixed a minor bug which occasionaly caused false negatives.
- Cleared up fix/hint generation and another nit following comments.
- Re-organized and added a couple tests.
- Made a note of this extension in documentation.
This check has been enabled internally as Facebook (pre-changes discussed above) for a bit over a year, with no reported issues.
It should be safe and ready to be upstreamed at this point.
Please let me know of any issues, let's get this landed!
Please don't use auto unless type is spelled explicitly in same statement or iterator.