Page MenuHomePhabricator

[clang-tidy] abseil-auto-make-unique
Needs RevisionPublic

Authored by hugoeg on Aug 16 2018, 10:30 AM.

Details

Summary

warns to use 'auto' to avoid repeating the type name and fixes the issue

Replace:

std::unique_ptr<Foo> x = make_unique<Foo>(...);

with:

auto x = make_unique<Foo>(...);

Diff Detail

Event Timeline

hugoeg created this revision.Aug 16 2018, 10:30 AM
hugoeg edited the summary of this revision. (Show Details)Aug 16 2018, 10:30 AM
hugoeg edited the summary of this revision. (Show Details)
  1. Please always upload all patches with full context.
  2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

what do you mean by "with full context"?

what do you mean by "with full context"?

-U99999 when generating the diff.

  1. Please always upload all patches with full context.
  2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

since this check checks for absl::make_unique primarily
if we move it to the general check, it'd be weird to check for absl::make_unique
right now our main goal is to release checks tailored specifically to abseil users, so if we can we would like to release this check separately in the abseil directory

  1. Please always upload all patches with full context.
  2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

since this check checks for absl::make_unique primarily
if we move it to the general check, it'd be weird to check for absl::make_unique

Why do you think it would be weird?
That list should/would be a user-configurable config option anyway.

right now our main goal is to release checks tailored specifically to abseil users, so if we can we would like to release this check separately in the abseil directory

Just checking, was there some memo i missed that abseil-related checks are exempt from all the usual guidelines?
Because this check is really not abseil-library-specific.

JonasToth retitled this revision from abseil-auto-make-unique to [clang-tidy} abseil-auto-make-unique.Aug 16 2018, 1:28 PM
JonasToth retitled this revision from [clang-tidy} abseil-auto-make-unique to [clang-tidy] abseil-auto-make-unique.
JonasToth set the repository for this revision to rCTE Clang Tools Extra.
  1. Please always upload all patches with full context.
  2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

I checked fast and modernize-use-auto does not catch this case.

Just checking, was there some memo i missed that abseil-related checks are exempt from all the usual guidelines?

Because this check is really not abseil-library-specific.

I agree that this check is general for make_... templates. It should be either an addition to modernize-use-auto or readability- or so. What we do for such cases, where the check is generally useful and specific to a guideline at the same time is aliasing.

You can take a look at hicpp- module where most checks are actually aliases.

JonasToth added inline comments.Aug 16 2018, 1:52 PM
clang-tidy/abseil/AutoMakeUniqueCheck.cpp
21

Please clang-format, return on next line.

23

clang::ast_matchers is used already at line 14. No need to add this line.

27

This sentence misses something. inbetween what?

34

The same rules apply for make_shared.

make_pair would be interesting as well, are there are standard make_ templates?

51

Formatting and Naming conventions.

I think the following is more readable:

if (const auto* Unique = dyn_cast<ClassTemplateSpecializationDecl>(var...)) {
  // stuff with var
}

return nullptr;
57

Naming conventions.

64

You don't need the ast_matchers namespace here.

Please follow the naming convention inside the function.

69

What exactly is the issue here in GCC? Please make this comment more explanatory and maybe add a FIXME to work around the issue (depending on the nature of the issue)

docs/ReleaseNotes.rst
63

Please add release notes.

docs/clang-tidy/checks/abseil-auto-make-unique.rst
11

Please add documentation that the 'Abstract Factory' use case is resolved correctly.

test/clang-tidy/abseil-auto-make-unique.cpp
34

How does the code look with multiple variable declarations?

Please add a test for

std::unique_ptr<int> x = absl::make_unique<int>(), y = absl::make_unique<int>(), z;
73

lets consider a 3 level class hierarchy.

struct A { virtual void Something(); };
struct B : A { void Something() override; };
struct C : B { void Something() override; };

std::unique_ptr<B> b_ptr = make_unique<B>(), c_ptr = make_unique<C>();
std::unique_ptr<B> c_ptr2 = make_unique<C>(), b_ptr2 = make_unique<B>();

What is the behaviour? I expect that these places break when transformed. To avoid you can check the VarDecl isSingleDecl() (or similar, i forgot the exact name) and only emit a fixit if it is.
Doing type transformations for the multi-definitions is tricky.

JonasToth added inline comments.Aug 17 2018, 12:26 AM
test/clang-tidy/abseil-auto-make-unique.cpp
73

isSingleDecl() is a member of DeclStmt.

zinovy.nis added a comment.EditedAug 18 2018, 12:14 PM
  1. Please always upload all patches with full context.
  2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

First of all, thanks, Hugo, for your check!

My +1 for extending modernize-use-auto as currently it doesn't support cases your check supports.
Extending because replacing

std::unique_ptr< int >y = std::make_unique<int>();

with

auto y = std::make_unique<int>();

is logically the same as replacing

long long int *y = new long long int (42);

with

auto* y = new long long int (42);

what modernize-use-auto does.

So your check should extend the existing one and not create one more check IMHO.

zinovy.nis added inline comments.Aug 18 2018, 12:43 PM
clang-tidy/abseil/AutoMakeUniqueCheck.cpp
21

auto first appeared in C++11, so you may use getLangOpts().CPlusPlus11
And std::make_unique is a part of C++14, so even `getLangOpts().CPlusPlus14.

66

You may move this definition after the next if+return.

docs/clang-tidy/checks/abseil-auto-make-unique.rst
10

MakeUnique<Foo>? Maybe std/abseil::make_unique?

lebedev.ri added inline comments.Aug 18 2018, 12:46 PM
clang-tidy/abseil/AutoMakeUniqueCheck.cpp
21

I agree on the auto part, but it really shouldn't care any further than that.
You can write custom yournamespace::MakeUnique<> in C++11.
(As i have already wrote, the list if MakeUnique functions-to-be-searched should be a config option.)

lebedev.ri requested changes to this revision.Dec 2 2018, 5:53 AM

Marking both of these as "changes requested" to highlight the similarity of issues in them.

This revision now requires changes to proceed.Dec 2 2018, 5:53 AM