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.
Details
Diff Detail
Event Timeline
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. |
Marking both of these as "changes requested" to highlight the similarity of issues in them.
Per the suggestions of reviewers, change the check so that it uses modernize-make-smart-pointer instead.
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. |
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. |
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. |
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? |
clang-tidy/abseil/MakeUniqueCheck.h | ||
---|---|---|
29 ↗ | (On Diff #177972) | Ah, so there is a check already, i missed that somehow. You should be adding an alias, and this isn't the right way to do so. |
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.
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.
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.
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
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.
Starting to look good i think.
docs/ReleaseNotes.rst | ||
---|---|---|
88 | Also add a new entry about the new 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; }; | |
94 | CanCallCtor, anyOf(unless(IgnoreListInit), unless(hasInitializationStyle(CXXNewExpr::ListInit)))) | |
104 | hasArgument(0, cxxNewExpr(CanCallCtor, anyOf(unless(IgnoreListInit), unless(hasInitializationStyle(CXXNewExpr::ListInit)))).bind(NewExpression)), |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
94 | I can't compile unless(IgnoreListInit), but unless(IgnoreListInit ? unless(anything()) : anything()) does work. I'm not sure how idiomatic that is though. |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
94 | Actually, unless(anything()) also does not compile. How should I make a matcher that won't trigger if IgnoreListInit is true? |
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 |
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 | ||
---|---|---|
9 | 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 | ||
10 | we have an implementation in Inputs/modernize-smart-ptr/unique_ptr.h, please use it. | |
60 | 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
|
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?
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
55 | You’re setting it false here. |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
55 | I thought that false was the default value if the options do not contain "IgnoreListInit"? |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
55 | Do you have a test that passes this option? I didn’t see one. |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
55 | 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. |
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... |
clang-tidy/modernize/MakeSmartPtrCheck.cpp | ||
---|---|---|
55 | Fixed by changing the Options value to "1" instead of true in the AbseilTidyModule |
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 is defined as typedef std::map<std::string, std::string> OptionMap;, so you need to assign a string, not a literal true value. hth...