This is an archive of the discontinued LLVM Phabricator instance.

Create modernize-make-unique check.
ClosedPublic

Authored by angelgarcia on Sep 25 2015, 10:03 AM.

Details

Summary

create a check that replaces 'std::unique_ptr<type>(new type(args...))' with 'std::make_unique<type>(args...)'. It was on the list of "Ideas for new Tools". It needs to be tested more carefully, but first I wanted to know if you think it is worth the effort.

Diff Detail

Event Timeline

angelgarcia retitled this revision from to Create modernize-make-unique check..
angelgarcia updated this object.
angelgarcia added a reviewer: alexfh.
angelgarcia added subscribers: klimek, cfe-commits.

This is definitely a useful check to have in modernize.

clang-tidy/modernize/MakeUniqueCheck.cpp
26–28

How can that ever return 0 if you have matchesName("::std::unique_ptr") below?

52

Why do we need the hasCanonicalType?

alexfh edited reviewers, added: klimek; removed: alexfh.Sep 26 2015, 4:47 PM
alexfh removed a subscriber: klimek.
angelgarcia marked an inline comment as done.

Remove 'hasCanonicalType'.

clang-tidy/modernize/MakeUniqueCheck.cpp
26–28

Maybe using macros or something like that. As I said, it needs to be more carefully tested.

klimek added inline comments.Sep 28 2015, 1:46 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
27–29

That should not be possible for the spelling location. Generally, I'd avoid writing code that doesn't fix a breaking test.

Two tests in which 'getTokenLength' returns 0.

klimek edited edge metadata.Sep 28 2015, 2:49 AM

Two tests in which 'getTokenLength' returns 0.

Thanks for the tests - question is: I would have expected us to use something like Lexer::getSourceText, which should give us the full range in the first test (do we not want to replace it?) and tell us that we don't have the type in one consecutive range in the second case.

How can Lexer::getSourceText give us the range? My problem precisely is
that I couldn't find any way to obtain the range of the constructor call
without the template arguments.

angelgarcia edited edge metadata.

I think this is a bit better than before.

klimek added inline comments.Sep 28 2015, 7:08 AM
clang-tidy/modernize/MakeUniqueCheck.cpp
75–76

Comment what cases this can happen in, and why we don't want to replace in those cases...

This raises a question. Do we want to do replacements when we use an alias
for std::unique_ptr? That fact that something is an unique_ptr might be an
implementation detail that should not be exposed, but it could also happen
that the alias is there only for brevity. What do you think?

This raises a question. Do we want to do replacements when we use an alias
for std::unique_ptr? That fact that something is an unique_ptr might be an
implementation detail that should not be exposed, but it could also happen
that the alias is there only for brevity. What do you think?

Good question. Generally, it seems to me that aliasing unique_ptr wouldn't seem useful apart from having a shortcut; unique_ptr is special in a couple of ways, which would make it particularly weird as an "implementation detail".

My take on it is that we want to replace those, until we find real use cases that warrant a more complex implementation.

Also replace on aliases.

klimek accepted this revision.Sep 29 2015, 2:21 AM
klimek edited edge metadata.

apart from the comment, LG

clang-tidy/modernize/MakeUniqueCheck.cpp
68–71

Please add a comment what this does (in which cases do we want to insert the type).

This revision is now accepted and ready to land.Sep 29 2015, 2:21 AM
angelgarcia edited edge metadata.

Add a comment.

angelgarcia closed this revision.Sep 29 2015, 2:38 AM