[clang-tidy] check for Abseil make_unique

[axzhang]

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.

[lebedev.ri]

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]

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.

[JonasToth]

In D55044#1312438, @lebedev.ri wrote:

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]

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

[axzhang]

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

[Eugene.Zelenko]

docs/ReleaseNotes.rst

93

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]

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]

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]

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]

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.

[hokein]

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.

[lebedev.ri]

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

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.

[axzhang]

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).

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.

[hokein]

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 D55044#1330102, @axzhang wrote:

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).

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]

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.

[lebedev.ri]

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

[lebedev.ri]

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]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

163

elide the braces

237

elide the braces

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

6

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

[axzhang]

Updated diff with full context.

[axzhang]

Make fixes to the UseLegacyFunction option, renaming to IgnoreListInit.

[lebedev.ri]

Starting to look good i think.

docs/ReleaseNotes.rst

88

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

[axzhang]

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

[lebedev.ri]

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)),

[axzhang]

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.

[axzhang]

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?

[lebedev.ri]

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]

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

[hokein]

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

• 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]

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]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

69

You’re setting it false here.

[axzhang]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

69

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

[hintonda]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

69

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

[hintonda]

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]

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]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

69

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

[axzhang]

Simplify tests and fix issues with Options.

[axzhang]

Pinging for visibility.

[alexfh]

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

[hokein]

In D55044#1589496, @alexfh wrote:

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.

[shafik]

Closing as it look like this is now modernize-make-unique

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../modernize/MakeUniqueCheck.h"
 #include "DurationAdditionCheck.h"
 #include "DurationComparisonCheck.h"
 #include "DurationConversionCastCheck.h"

         "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");
+    CheckFactories.registerCheck<modernize::MakeUniqueCheck>(
+        "abseil-make-unique");
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+    Opts["abseil-make-unique.MakeSmartPtrFunctionHeader"] = "absl/memory/memory.h";
+    Opts["abseil-make-unique.MakeSmartPtrFunction"] = "absl::make_unique";
+    Opts["abseil-make-unique.UseLegacyFunction"] = true;
+    return Options;
   }
 };
 

clang-tidy/modernize/MakeSmartPtrCheck.h

   const std::string MakeSmartPtrFunctionHeader;
   const std::string MakeSmartPtrFunctionName;
   const bool IgnoreMacros;
+  const bool UseLegacyFunction;
 
   void checkConstruct(SourceManager &SM, ASTContext *Ctx,
                       const CXXConstructExpr *Construct, const QualType *Type,

clang-tidy/modernize/MakeSmartPtrCheck.cpp

           Options.get("MakeSmartPtrFunctionHeader", StdMemoryHeader)),
       MakeSmartPtrFunctionName(
           Options.get("MakeSmartPtrFunction", MakeSmartPtrFunctionName)),
-      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
+      IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+      UseLegacyFunction(Options.getLocalOrGlobal("UseLegacyFunction", false)) {}
 
 void MakeSmartPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", IncludeStyle);
   Options.store(Opts, "MakeSmartPtrFunctionHeader", MakeSmartPtrFunctionHeader);
   Options.store(Opts, "MakeSmartPtrFunction", MakeSmartPtrFunctionName);
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+  Options.store(Opts, "UseLegacyFunction", UseLegacyFunction);
 }
 
 bool MakeSmartPtrCheck::isLanguageVersionSupported(

   if (Invalid)
     return;
 
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == CXXNewExpr::ListInit) {
+    return;
+  }
   auto Diag = diag(ConstructCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 

     return;
   }
 
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == CXXNewExpr::ListInit) {
+    return;
+  }
   auto Diag = diag(ResetCallStart, "use %0 instead")
               << MakeSmartPtrFunctionName;
 

docs/ReleaseNotes.rst

   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
+- New alias :doc:`abseil-make-unique
+  <clang-tidy/checks/abseil-make-unique>` to :doc:`modernize-make-unique
+  <clang-tidy/checks/modernize-make-unique>`
+  added.
+
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 

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

+.. title:: clang-tidy - abseil-make-unique
+.. meta::
+   :http-equiv=refresh: 5;URL=abseil-make-unique.html
+
+abseil-make-unique
+============
+
+The abseil-make-unique check is an alias, please see
+`modernize-make-unique <modernize-make-unique.html>`_ for more information.

docs/clang-tidy/checks/list.rst

    abseil-duration-subtraction
    abseil-duration-unnecessary-conversion
    abseil-faster-strsplit-delimiter
+   abseil-make-unique (redirects to modernize-make-unique) <modernize-make-unique>
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls

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

 
    If set to non-zero, the check will not give warnings inside macros. Default
    is `1`.
+
+.. option:: UseLegacyFunction
+   
+   If set to non-zero, the check will ignore features from C++11 and later such
+   as list initialization. Default is `0`. 

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

+// RUN: %check_clang_tidy %s abseil-make-unique %t -- -- -std=c++11 \
+
+namespace std {
+
+template <typename T>
+class default_delete {};
+
+template <typename type, typename Deleter = std::default_delete<type>>
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr<type> &t) = delete;
+  unique_ptr(unique_ptr<type> &&t) {}
+  ~unique_ptr() {}
+  type &operator*() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr &operator=(unique_ptr &&) { return *this; }
+  template <typename T>
+  unique_ptr &operator=(unique_ptr<T> &&) { return *this; }
+
+private:
+  type *ptr;
+};
+
+}  // namespace std
+
+class A {
+ int x;
+ int y;
+
+ public:
+   A(int _x, int _y): x(_x), y(_y) {}
+};
+
+struct Base {
+  Base();
+};
+
+struct Derived : public Base {
+  Derived();
+};
+
+int* returnPointer();
+void expectPointer(std::unique_ptr<int> p);
+
+std::unique_ptr<int> makeAndReturnPointer() {
+  return std::unique_ptr<int>(new int(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: return absl::make_unique<int>(0);
+}
+
+void Positives() {
+  std::unique_ptr<int> P1 = std::unique_ptr<int>(new int(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr<int> P1 = absl::make_unique<int>(1);
+
+  P1.reset(new int(2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P1 = absl::make_unique<int>(2);
+
+  // Non-primitive paramter
+  std::unique_ptr<A> P2 = std::unique_ptr<A>(new A(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr<A> P2 = absl::make_unique<A>(1, 2);
+
+  P2.reset(new A(3, 4));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P2 = absl::make_unique<A>(3, 4);
+
+  // No arguments to new expression
+  std::unique_ptr<int> P3 = std::unique_ptr<int>(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr<int> P3 = absl::make_unique<int>();
+
+  P3.reset(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P3 = absl::make_unique<int>();
+
+  // Nested parentheses
+  std::unique_ptr<int> P4 = std::unique_ptr<int>((new int(3)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: std::unique_ptr<int> P4 = absl::make_unique<int>(3);
+
+  P4 = std::unique_ptr<int>(((new int(4))));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique<int>(4);
+
+  P4.reset((new int(5)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: P4 = absl::make_unique<int>(5);
+
+  // With auto
+  auto P5 = std::unique_ptr<int>(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: auto P5 = absl::make_unique<int>();
+
+  {
+    // No std
+    using namespace std;
+    unique_ptr<int> Q = unique_ptr<int>(new int());
+    // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use absl::make_unique instead [abseil-make-unique]
+    // CHECK-FIXES: unique_ptr<int> Q = absl::make_unique<int>();
+
+    Q = unique_ptr<int>(new int());
+    // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use absl::make_unique instead [abseil-make-unique]
+    // CHECK-FIXES: Q = absl::make_unique<int>();
+  }
+
+  // Create the unique_ptr as a parameter to a function
+  expectPointer(std::unique_ptr<int>(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: use absl::make_unique instead [abseil-make-unique]
+  // CHECK-FIXES: expectPointer(absl::make_unique<int>());
+}
+
+void Negatives() {
+  // Only warn if explicitly allocating a new object
+  std::unique_ptr<int> R = std::unique_ptr<int>(returnPointer());
+  R.reset(returnPointer());
+
+  // Only replace if the template type is same as new type
+  auto Pderived = std::unique_ptr<Base>(new Derived());
+}