Page MenuHomePhabricator

[clang-tidy] check for Abseil make_unique
AcceptedPublic

Authored by axzhang on Nov 28 2018, 8:14 PM.

Details

Summary

Add a new check for https://abseil.io/tips/126, to detect uses of constructing a std::unique_ptr with a call to new, and recommend substituting with absl::make_unique or absl::WrapUnique. Note that some functionality may overlap with existing checks to substitute with std::make_unique, but this check differs in that it is specific to Abseil, and in how it utilizes both absl::make_unique and absl::WrapUnique.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thanks for working on this.
Semi-duplicate of D50852 Please see discussion there.
It should not be an abseil-specific check, the prefix (std,abseil), and the function (make_unique) should be a config option, and it should likely be a part of modernize-use-auto.

Eugene.Zelenko retitled this revision from [cfe-dev] clang-tidy check for Abseil make_unique to [clang-tidy] check for Abseil make_unique.Nov 29 2018, 10:24 AM
Eugene.Zelenko edited reviewers, added: alexfh, hokein, aaron.ballman, JonasToth; removed: klimek, EricWF.
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
70

Please use alphabetical order for new checks.

docs/clang-tidy/checks/abseil-make-unique.rst
7

Please synchronize with Release Notes.

Thanks for working on this.
Semi-duplicate of D50852 Please see discussion there.
It should not be an abseil-specific check, the prefix (std,abseil), and the function (make_unique) should be a config option, and it should likely be a part of modernize-use-auto.

I agree here. Instead of reimplementing the functionality we should rather refactor the existing check and make it flexible. It is possible to register the absl version of it with a different default-configuration so the refactoring will be the correct one.

test/clang-tidy/abseil-make-unique.cpp
44

All your test cases seem to create only a single unique_ptr, please add cases like

std::unique_ptr<int> b1(new int(32)), b2(new int(42));

From my experience in rewriting decls, they should be excluded. It is very cumbersome to get it right in all cases (think pointers, pointer to pointers, const, ...) and readability-isolcate-declaration exists to split variable declarations first.

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
axzhang updated this revision to Diff 177972.Dec 12 2018, 4:28 PM

Per the suggestions of reviewers, change the check so that it uses modernize-make-smart-pointer instead.

Eugene.Zelenko added inline comments.Dec 12 2018, 4:37 PM
docs/ReleaseNotes.rst
73

Please use double `, not single ones to hightlight language constructs. Same in documentation.

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

Please use .. code-block:: c++. See other checks documentation as example.

20

Please use hyperlink like:

The Abseil Style Guide <https://abseil.io/tips/126>_ discusses this issue in more detail.

EricWF added inline comments.Dec 12 2018, 6:03 PM
clang-tidy/abseil/MakeUniqueCheck.cpp
28 ↗(On Diff #177972)

Does this catch std::__1::unique_ptr, where __1 is an inline namespace? (Either way, we should add a test)

Maybe it's better to first filter declarations using an isInStdNamespace matcher and then check for the name unique_ptr.

Eugene.Zelenko added inline comments.Dec 12 2018, 6:23 PM
clang-tidy/abseil/MakeUniqueCheck.cpp
28 ↗(On Diff #177972)

Tests for other checks don't contain libc++ specific, so if problems existed they were resolved long time ago.

EricWF marked an inline comment as done.Dec 12 2018, 7:13 PM

I'm not sure how you're building this, but it doesn't link for me. We need to add a dependency on clangTidyModernizeModule since we're deriving from its MakeSmartPtrCheck.

clang-tidy/abseil/MakeUniqueCheck.cpp
28 ↗(On Diff #177972)

Using versioning namespaces is not specific to libc++. But, as you suggested, this case appears to work.

I also see that this is just a copy of modernize/MakeUniqueCheck.cpp. Can we find a way to share the matcher between these two implementations?

lebedev.ri requested changes to this revision.Dec 12 2018, 10:11 PM
lebedev.ri added inline comments.
clang-tidy/abseil/MakeUniqueCheck.h
29 ↗(On Diff #177972)

Ah, so there is a check already, i missed that somehow.
If the modernize::MakeSmartPtrCheck is not sufficient to the needs, it needs to be extended.

You should be adding an alias, and this isn't the right way to do so.
E.g. see D53771 on how to do that.

This revision now requires changes to proceed.Dec 12 2018, 10:11 PM

In fact, the existing modernize-make-unique can be configured to support absl::make_unique, you'd just need to configure the check option MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).

The biggest missing part of the modernize-make-unique is absl::WrapUnique, I think we should extend MakeSmartPtrCheck class (maybe add hooks) to support it.

In fact, the existing modernize-make-unique can be configured to support absl::make_unique, you'd just need to configure the check option MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).

See https://reviews.llvm.org/D52670#change-eVDYJhWSHWeW (the getModuleOptions() part) on how to do that

The biggest missing part of the modernize-make-unique is absl::WrapUnique, I think we should extend MakeSmartPtrCheck class (maybe add hooks) to support it.

In fact, the existing modernize-make-unique can be configured to support absl::make_unique, you'd just need to configure the check option MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).

The biggest missing part of the modernize-make-unique is absl::WrapUnique, I think we should extend MakeSmartPtrCheck class (maybe add hooks) to support it.

What is the best way to extend MakeSmartPtrCheck? The behavior I want to achieve is that absl::WrapUnique is suggested when brace initialization is used, but absl::make_unique is used in all other cases.

In D55044#1329604, @hokein wrote:
In fact, the existing modernize-make-unique can be configured to support absl::make_unique, you'd just need to configure the check option MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).

See https://reviews.llvm.org/D52670#change-eVDYJhWSHWeW (the getModuleOptions() part) on how to do that

Thanks! Yes, it is trivial to create an alias check, but if we want to support WrapUnique, I don't think this is a right way to go.

In fact, the existing modernize-make-unique can be configured to support absl::make_unique, you'd just need to configure the check option MakeSmartPtrFunction to absl::make_unique (this is what we do inside google).

The biggest missing part of the modernize-make-unique is absl::WrapUnique, I think we should extend MakeSmartPtrCheck class (maybe add hooks) to support it.

What is the best way to extend MakeSmartPtrCheck? The behavior I want to achieve is that absl::WrapUnique is suggested when brace initialization is used, but absl::make_unique is used in all other cases.

I think we'd need to add more interfaces to MakeSmartPtrCheck allowing subclasses to inject their logic to handle different initialization styles of new expression.

  • MakeSmartPtrCheck::replaceNew is the interesting place
  • we may split MakeSmartPtrCheck::replaceNew implementation into different methods (e.g. handleCallInit, handleListInit) which are corresponding to handle different new cases
  • from my experience, handling brace initialization is tricky, there are lots of corner cases, you can see the comments in MakeSmartPtrCheck::replaceNew
axzhang updated this revision to Diff 190559.Mar 13 2019, 7:56 PM

Apologies for the extended hiatus. I have changed the implementation to an alias to modernize-make-unique, and have added an extra option to modernize-make-unique that ignores C++11 features such as list initializations, to allow absl::make_unique to function correctly. Also, after looking more into absl::WrapUnique, it seems like this also is not really the function to use for brace initializations, so no extra functionality needs to be added to the existing modernize-make-unique check.

Please always upload all patches with full context (=U99999)

lebedev.ri added inline comments.Mar 14 2019, 12:17 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
55

I'm not sure it makes sense to look for global UseLegacyFunction?
It doesn't sound all that common.

docs/clang-tidy/checks/modernize-make-unique.rst
52

Name is too cryptic i'd say.
Maybe just IgnoreInitListExprs

MyDeveloperDay added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
163

elide the braces

237

elide the braces

docs/clang-tidy/checks/abseil-make-unique.rst
7

the ==== length needs to match the text above it

axzhang updated this revision to Diff 190676.Mar 14 2019, 11:03 AM

Updated diff with full context.

axzhang updated this revision to Diff 190775.Mar 14 2019, 8:09 PM

Make fixes to the UseLegacyFunction option, renaming to IgnoreListInit.

Starting to look good i think.

docs/ReleaseNotes.rst
88

Also add a new entry about the new option for modernize-make-unique.

axzhang updated this revision to Diff 192394.Mar 26 2019, 6:05 PM

Add documentation about the added option for modernize-make-unique.

It is best to error-out early.
Could you please try this instead:

clang-tidy/modernize/MakeSmartPtrCheck.cpp
39
AST_MATCHER_P(CXXNewExpr, hasInitializationStyle,
              CXXNewExpr::InitializationStyle, IS) {
  return Node.getInitializationStyle() == IS;
};
108–111
CanCallCtor, anyOf(unless(IgnoreListInit), unless(hasInitializationStyle(CXXNewExpr::ListInit))))
121–125
hasArgument(0, cxxNewExpr(CanCallCtor, anyOf(unless(IgnoreListInit), unless(hasInitializationStyle(CXXNewExpr::ListInit)))).bind(NewExpression)),
axzhang marked an inline comment as done.Apr 1 2019, 7:37 PM
axzhang added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
108–111

I can't compile unless(IgnoreListInit), but unless(IgnoreListInit ? unless(anything()) : anything()) does work. I'm not sure how idiomatic that is though.

axzhang marked 2 inline comments as done and an inline comment as not done.Apr 1 2019, 7:41 PM
axzhang added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
108–111

Actually, unless(anything()) also does not compile. How should I make a matcher that won't trigger if IgnoreListInit is true?

lebedev.ri resigned from this revision.Apr 4 2019, 11:49 PM

Hmm, i suppose this looks good to me now.
It would be good to early-exit, but i'm not sure how important that is.
Please see D52771 / clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
on how to use the bool params to short-circuit the astmatchers.

docs/ReleaseNotes.rst
114

... which does xyz

axzhang updated this revision to Diff 194435.Apr 9 2019, 6:50 PM
axzhang marked an inline comment as not done.

If no more changes need to be made, is this okay to be merged in?

Thanks, looks better now.
I saw there are still a few undone comments, please mark them done when you address them.

docs/clang-tidy/checks/abseil-make-unique.rst
8

This is not really alias, abseil-make-unique is different than modernize-make-unique, please add more details to the document here.

20

This comment is undone.

test/clang-tidy/abseil-make-unique.cpp
9

we have an implementation in Inputs/modernize-smart-ptr/unique_ptr.h, please use it.

59

since the check shares the underlying modernize-make-unique implementation, I think there is no need to repeat most test cases here (those are covered in test/clang-tidy/modernize-make-unique.cpp).

I'd suggest

  • add a simple test to verify this check provides abseil-related fixes (abseil header, absl::make_unique);
  • adding test cases for list initialization (make sure we ignore those in this check);
axzhang marked an inline comment as done.Apr 17 2019, 8:05 PM

I'm having some issues with the AbseilTidyModule options. I am storing IgnoreListInit as true for the abseil-make-unique check, but when I run the check it shows IgnoreListInit as being false. Any ideas why this is happening?

hintonda added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
69

You’re setting it false here.

axzhang marked an inline comment as done.Apr 17 2019, 8:27 PM
axzhang added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
69

I thought that false was the default value if the options do not contain "IgnoreListInit"?

hintonda added inline comments.Apr 17 2019, 8:51 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
69

Do you have a test that passes this option? I didn’t see one.

hintonda added inline comments.Apr 17 2019, 9:10 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
69

I could be wrong, but when you do an Options.get(), that looks at what was passed on the commandline, and if it wasn't found, it sets the default provided.

hintonda added inline comments.Apr 18 2019, 11:16 AM
clang-tidy/abseil/AbseilTidyModule.cpp
77

This is defined as typedef std::map<std::string, std::string> OptionMap;, so you need to assign a string, not a literal true value. hth...

axzhang marked 3 inline comments as done.Apr 18 2019, 11:25 AM
axzhang added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
69

Fixed by changing the Options value to "1" instead of true in the AbseilTidyModule

axzhang updated this revision to Diff 195791.Apr 18 2019, 11:25 AM

Simplify tests and fix issues with Options.

axzhang marked an inline comment as done.Apr 23 2019, 6:26 PM
axzhang marked 20 inline comments as done.Apr 24 2019, 4:15 PM

Pinging for visibility.

Seems good now. Haojian, do you have any concerns?

hokein accepted this revision.Jul 18 2019, 5:21 AM

Seems good now. Haojian, do you have any concerns?

sorry for the looong delay, I totally miss this thread. looks good from my side, you'd need to rebase the patch to master.

docs/clang-tidy/checks/abseil-make-unique.rst
23

I think we should expose the MakeSmartPtrFunctionHeader to the document, this option is used to configure the absl memory header.

test/clang-tidy/abseil-make-unique.cpp
2

nit: drop the -std=c++11, check_clang_tidy.py will add it by default.

This revision is now accepted and ready to land.Jul 18 2019, 5:21 AM