Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
260 msx64 debian > Clang Tools.clang-tidy/checkers::modernize-use-emplace.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python3.8 /mnt/disks/ssd0/agent/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/llvm-project/clang-tools-extra/test/clang-tidy/checkers/modernize-use-emplace.cpp modernize-use-emplace /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-use-emplace.cpp.tmp -- -config="{CheckOptions: [{key: modernize-use-emplace.ContainersWithPushBack, value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, {key: modernize-use-emplace.TupleTypes, value: '::std::pair; std::tuple; ::test::Single'}, {key: modernize-use-emplace.TupleMakeFunctions, value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] }"
390 msx64 windows > Clang Tools.clang-tidy/checkers::modernize-use-emplace.cpp
Script: -- : 'RUN: at line 1'; C:/Python39/python.exe C:/ws/w16c2-2/llvm-project/premerge-checks/clang-tools-extra/test/../test\clang-tidy\check_clang_tidy.py C:\ws\w16c2-2\llvm-project\premerge-checks\clang-tools-extra\test\clang-tidy\checkers\modernize-use-emplace.cpp modernize-use-emplace C:\ws\w16c2-2\llvm-project\premerge-checks\build\tools\clang\tools\extra\test\clang-tidy\checkers\Output\modernize-use-emplace.cpp.tmp -- -config="{CheckOptions: [{key: modernize-use-emplace.ContainersWithPushBack, value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, {key: modernize-use-emplace.TupleTypes, value: '::std::pair; std::tuple; ::test::Single'}, {key: modernize-use-emplace.TupleMakeFunctions, value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] }"

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
251

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.

271

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

297

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
297

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.