This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add proper emplace checks to modernize-use-emplace
ClosedPublic

Authored by nicovank on Apr 28 2021, 9:46 AM.

Details

Summary

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.

Diff Detail

Event Timeline

nicovank created this revision.Apr 28 2021, 9:46 AM
nicovank requested review of this revision.Apr 28 2021, 9:46 AM

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"?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
25

Please don't use auto unless type is spelled explicitly in same statement or iterator.

39

Please don't use auto unless type is spelled explicitly in same statement or iterator.

nicovank updated this revision to Diff 341342.Apr 28 2021, 3:58 PM

Fix a couple variable names and explicitely specify a couple types

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.

nicovank updated this revision to Diff 341349.Apr 28 2021, 4:26 PM

Explicitely specify one more type

nicovank marked 3 inline comments as done.Apr 28 2021, 4:31 PM

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.

Eugene.Zelenko added inline comments.Apr 28 2021, 5:18 PM
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.

kuhar added inline comments.Apr 28 2021, 6:12 PM
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?

nicovank updated this revision to Diff 343399.May 6 2021, 7:18 AM
nicovank marked 7 inline comments as done.

Fix some style comments

nicovank added inline comments.May 6 2021, 7:23 AM
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.

kuhar resigned from this revision.May 16 2022, 10:59 AM
kuhar added inline comments.
clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
44

nit: StringRef( seems unnecessary on the rhs of =

Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 10:59 AM
nicovank updated this revision to Diff 431387.May 23 2022, 8:33 AM

Update!

  1. Rebased.
  2. Fixed a minor bug which occasionaly caused false negatives.
  3. Cleared up fix/hint generation and another nit following comments.
  4. Re-organized and added a couple tests.
  5. 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!

nicovank marked 2 inline comments as done.May 23 2022, 8:33 AM
nicovank updated this revision to Diff 431426.May 23 2022, 10:51 AM

Fix formatting issues

kuhar accepted this revision.May 25 2022, 7:32 AM
kuhar added a subscriber: kuhar.

LGTM but please get a second approval before submitting.

This revision is now accepted and ready to land.May 25 2022, 7:32 AM
ivanmurashko accepted this revision.EditedJun 2 2022, 2:40 PM

LGTM, thank you a lot for the useful modification!