[clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Prazek]

Summary

Bugfix for 27321. When the constructor of stored pointer
type is private then it is invalid to change it to
make_shared or make_unique.

[Prazek]

Added note in release notes

[Prazek]

I hate it when arc do this thing. When origin/master is not the trunk...

[aaron.ballman]

Good catch! Some small nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

32–33 ↗

(On Diff #67460)

Nit: this reads a bit strangely, how about: Calling make_smart_ptr from within a member function of a type with a private constructor would be ill-formed.

test/clang-tidy/modernize-make-shared.cpp

109 ↗

(On Diff #67460)

Add comments explaining why make_shared is not correct. Also, please add a case showing that protected constructors still get the proper fix applied.

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

112 ↗

(On Diff #67460)

Add comments as above.

[Prazek]

test/clang-tidy/modernize-make-shared.cpp

109 ↗

(On Diff #67460)

You mean I should also not warn when the constructor is protected? Make sense.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

78 ↗

(On Diff #67460)

I think will be better to have Fixed bugs: section as we had in 3.9 release notes.

[aaron.ballman]

test/clang-tidy/modernize-make-shared.cpp

109 ↗

(On Diff #67460)

Correct, protected constructors should be handled like private constructors.

[Prazek]

• fixes

[Prazek]

• fixes

[aaron.ballman]

LGTM with one minor nit (you can fix it during the commit).

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

Perhaps: unless(isPublic()) instead of anyOf(isPrivate(), isProtected())?

[Prazek]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

POD types doesn't have public constructors so it will fail :)

[aaron.ballman]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

Don't they have an implicit one for the purposes of a CXXConstructExpr? Looking at an AST dump for:

struct S {
  int i;
};

yields:

|-CXXRecordDecl 0x26d74ae5348 <line:25:1, line:27:1> line:25:8 referenced struct S definition
| |-CXXRecordDecl 0x26d74ae5460 <col:1, col:8> col:8 implicit struct S
| |-FieldDecl 0x26d74ae7880 <line:26:3, col:7> col:7 i 'int'
| |-CXXConstructorDecl 0x26d74ae87b8 <line:25:8> col:8 implicit used S 'void (void) noexcept' inline
| | `-CompoundStmt 0x26d74ae3850 <col:8>
| |-CXXConstructorDecl 0x26d74ae34a8 <col:8> col:8 implicit constexpr S 'void (const struct S &)' inline noexcept-unevaluated 0x26d74ae34a8
| | `-ParmVarDecl 0x26d74ae35e0 <col:8> col:8 'const struct S &'
| `-CXXConstructorDecl 0x26d74ae3678 <col:8> col:8 implicit constexpr S 'void (struct S &&)' inline noexcept-unevaluated 0x26d74ae3678
|   `-ParmVarDecl 0x26d74ae37b0 <col:8> col:8 'struct S &&'

[Prazek]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

what about std::shared_ptr<int> x = std::shared_ptr(new int); ?
If I recall correctly, it didn't work on this test.

[alexfh]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

There's no CXXConstructExpr for primitive types:

$ cat /tmp/qq.cc 
#include <memory>

struct S { S(); };
void ffff() {
  std::shared_ptr<int> p1 = std::shared_ptr<int>(new int);
  std::shared_ptr<S> p2 = std::shared_ptr<S>(new S);
}
$ clang-check -ast-dump -ast-dump-filter=ffff /tmp/qq.cc -- -std=c++11
Dumping ffff:
FunctionDecl 0x7f48270fb6e0 </tmp/qq.cc:4:1, line:7:1> line:4:6 ffff 'void (void)'
`-CompoundStmt 0x7f48271582a8 <col:13, line:7:1>
  |-DeclStmt 0x7f4827131000 <line:5:3, col:58>
  | `-VarDecl 0x7f48270fb9c8 <col:3, col:57> col:24 p1 'std::shared_ptr<int>':'class std::shared_ptr<int>' cinit
  |   `-ExprWithCleanups 0x7f4827130fe8 <col:29, col:57> 'std::shared_ptr<int>':'class std::shared_ptr<int>'
  |     `-CXXConstructExpr 0x7f4827130fb0 <col:29, col:57> 'std::shared_ptr<int>':'class std::shared_ptr<int>' 'void (class std::shared_ptr<int> &&) noexcept' elidable
  |       `-MaterializeTemporaryExpr 0x7f4827130f98 <col:29, col:57> 'class std::shared_ptr<int>' xvalue
  |         `-CXXFunctionalCastExpr 0x7f48271303c8 <col:29, col:57> 'std::shared_ptr<int>':'class std::shared_ptr<int>' functional cast to std::shared_ptr<int> <ConstructorConversion>
  |           `-CXXBindTemporaryExpr 0x7f48271303a8 <col:29, col:57> 'std::shared_ptr<int>':'class std::shared_ptr<int>' (CXXTemporary 0x7f48271303a0)
  |             `-CXXConstructExpr 0x7f4827130338 <col:29, col:57> 'std::shared_ptr<int>':'class std::shared_ptr<int>' 'void (int *)'
  |               `-CXXNewExpr 0x7f48270fbb58 <col:50, col:54> 'int *' Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
  `-DeclStmt 0x7f4827158290 <line:6:3, col:52>
    `-VarDecl 0x7f4827131238 <col:3, col:51> col:22 p2 'std::shared_ptr<S>':'class std::shared_ptr<struct S>' cinit
      `-ExprWithCleanups 0x7f4827158278 <col:27, col:51> 'std::shared_ptr<S>':'class std::shared_ptr<struct S>'
        `-CXXConstructExpr 0x7f4827158240 <col:27, col:51> 'std::shared_ptr<S>':'class std::shared_ptr<struct S>' 'void (class std::shared_ptr<struct S> &&) noexcept' elidable
          `-MaterializeTemporaryExpr 0x7f4827158228 <col:27, col:51> 'class std::shared_ptr<struct S>' xvalue
            `-CXXFunctionalCastExpr 0x7f4827157658 <col:27, col:51> 'std::shared_ptr<S>':'class std::shared_ptr<struct S>' functional cast to std::shared_ptr<S> <ConstructorConversion>
              `-CXXBindTemporaryExpr 0x7f4827157638 <col:27, col:51> 'std::shared_ptr<S>':'class std::shared_ptr<struct S>' (CXXTemporary 0x7f4827157630)
                `-CXXConstructExpr 0x7f48271575c8 <col:27, col:51> 'std::shared_ptr<S>':'class std::shared_ptr<struct S>' 'void (struct S *)'
                  `-CXXNewExpr 0x7f4827131838 <col:46, col:50> 'struct S *' Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
                    `-CXXConstructExpr 0x7f4827131808 <col:50> 'struct S' 'void (void)'

[malcolm.parsons]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

32–33 ↗

(On Diff #67967)

The private constructor could also be called from a friend class.

[alexfh]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

32–33 ↗

(On Diff #67967)

It might only be relevant, if std::make_(shared|unique) is declared as a friend of the class in question, which is unlikely to be a common case and I don't think we need to focus on this now.

[Prazek]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

32–33 ↗

(On Diff #67967)

I am totally aware of it, but I never saw a friend delcaration for any class/function from libc++.

[Prazek]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

exactly.

[alexfh]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

But this means that unless(isPublic()) should work, IIUC.

[Prazek]

clang-tidy/modernize/MakeSmartPtrCheck.cpp

35 ↗

(On Diff #67967)

Oh I see.
Yea I remember right now what I did:
has(ignoringImpCasts(cxxConstructExpr(

hasDeclaration(decl(isPublic())))))))

This ofc not gonna work, but

unless(has(ignoringImpCasts(cxxConstructExpr(

hasDeclaration(decl(unless(isPublic())))))))

should be totally fine

[Prazek]

• fixes

[alexfh]

LG. Thank you!

[Prazek]

Sorry for long delay. I had some issues with git-svn on mac.

clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp

MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name, ClangTidyContext *Context,
                                      std::string makeSmartPtrFunctionName)
     : ClangTidyCheck(Name, Context),
       makeSmartPtrFunctionName(std::move(makeSmartPtrFunctionName)) {}
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+      hasDeclaration(decl(unless(isPublic())))))));
+
   Finder->addMatcher(
       cxxBindTemporaryExpr(has(ignoringParenImpCasts(
           cxxConstructExpr(
               hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
               hasArgument(0,
                           cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
-                                         equalsBoundNode(PointerType))))))
+                                         equalsBoundNode(PointerType))))),
+                                     CanCallCtor)
                               .bind(NewExpression)))
               .bind(ConstructorCall)))),
       this);
 }
 
 void MakeSmartPtrCheck::check(const MatchFinder::MatchResult &Result) {
   // 'smart_ptr' refers to 'std::shared_ptr' or 'std::unique_ptr' or other
   // pointer, 'make_smart_ptr' refers to 'std::make_shared' or

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `performance-inefficient-string-concatenation
   the ``operator+``, instead of ``operator+=``.
 
 - New `readability-non-const-parameter
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-non-const-parameter.html>`_ check
 
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+Fixed bugs:
+- `modernize-make-unique
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html>`_
+  and `modernize-make-shared
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_
+  Calling ``make_{unique|shared}`` from within a member function of a type
+  with a private or protected constructor would be ill-formed.
+
 Improvements to include-fixer
 -----------------------------
 
 - Emacs integration was added.
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp

void basic() {
   // This emulates std::move.
   std::shared_ptr<int> Move = static_cast<std::shared_ptr<int> &&>(P1);
 
   // Placemenet arguments should not be removed.
   int *PInt = new int;
   std::shared_ptr<int> Placement = std::shared_ptr<int>(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+    auto callsPublic = std::shared_ptr<Private>(new Private);
+    // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+    // CHECK-FIXES: auto callsPublic = std::make_shared<Private>();
+    auto ptr = std::shared_ptr<Private>(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+    auto callsPublic = std::shared_ptr<Protected>(new Protected(1, 2));
+    // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+    // CHECK-FIXES: auto callsPublic = std::make_shared<Protected>(1, 2);
+    auto ptr = std::shared_ptr<Protected>(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
   // Direct initialization with parenthesis.
   std::shared_ptr<DPair> PDir1 = std::shared_ptr<DPair>(new DPair(1, T));
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use std::make_shared instead
   // CHECK-FIXES: std::shared_ptr<DPair> PDir1 = std::make_shared<DPair>(1, T);
 

clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

void basic() {
   // This emulates std::move.
   std::unique_ptr<int> Move = static_cast<std::unique_ptr<int>&&>(P1);
 
   // Placemenet arguments should not be removed.
   int *PInt = new int;
   std::unique_ptr<int> Placement = std::unique_ptr<int>(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+    auto callsPublic = std::unique_ptr<Private>(new Private);
+    // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+    // CHECK-FIXES: auto callsPublic = std::make_unique<Private>();
+    auto ptr = std::unique_ptr<Private>(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+    auto callsPublic = std::unique_ptr<Protected>(new Protected(1, 2));
+    // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+    // CHECK-FIXES: auto callsPublic = std::make_unique<Protected>(1, 2);
+    auto ptr = std::unique_ptr<Protected>(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
   // Direct initialization with parenthesis.
   std::unique_ptr<DPair> PDir1 = std::unique_ptr<DPair>(new DPair(1, T));
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<DPair> PDir1 = std::make_unique<DPair>(1, T);