This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Prazek on Aug 9 2016, 9:57 PM.

Details

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.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 67458.Aug 9 2016, 9:57 PM
Prazek retitled this revision from to [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix.
Prazek updated this object.
Prazek added reviewers: alexfh, aaron.ballman, hokein.
Prazek added a subscriber: cfe-commits.
Prazek updated this revision to Diff 67459.Aug 9 2016, 10:05 PM

Added note in release notes

Prazek updated this revision to Diff 67460.Aug 9 2016, 10:07 PM

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

aaron.ballman edited edge metadata.Aug 10 2016, 5:03 AM

Good catch! Some small nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp
32–33

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

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

Add comments as above.

Prazek added inline comments.Aug 10 2016, 10:02 AM
test/clang-tidy/modernize-make-shared.cpp
109

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

Eugene.Zelenko added inline comments.
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 added inline comments.Aug 10 2016, 11:33 AM
test/clang-tidy/modernize-make-shared.cpp
109

Correct, protected constructors should be handled like private constructors.

Prazek updated this revision to Diff 67966.Aug 13 2016, 7:15 PM
Prazek marked 6 inline comments as done.
Prazek edited edge metadata.
  • fixes
Prazek updated this revision to Diff 67967.Aug 13 2016, 7:16 PM
  • fixes
aaron.ballman accepted this revision.Aug 16 2016, 6:50 AM
aaron.ballman edited edge metadata.

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

clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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

This revision is now accepted and ready to land.Aug 16 2016, 6:50 AM
Prazek added inline comments.Aug 16 2016, 3:37 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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

aaron.ballman added inline comments.Aug 16 2016, 3:47 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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 added inline comments.Aug 16 2016, 8:46 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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

alexfh added inline comments.Aug 17 2016, 2:56 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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 added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
32–33

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

alexfh added inline comments.Aug 17 2016, 9:34 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
32–33

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 added inline comments.Aug 17 2016, 9:37 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
32–33

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

Prazek marked 8 inline comments as done.Aug 17 2016, 9:38 AM
Prazek added inline comments.
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

exactly.

Prazek marked 2 inline comments as done.Aug 17 2016, 9:38 AM
alexfh added inline comments.Aug 17 2016, 9:49 AM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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

Prazek added inline comments.Aug 19 2016, 8:40 PM
clang-tidy/modernize/MakeSmartPtrCheck.cpp
35

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 updated this revision to Diff 68824.Aug 21 2016, 10:16 PM
Prazek edited edge metadata.
  • fixes
alexfh accepted this revision.Aug 22 2016, 10:29 AM
alexfh edited edge metadata.

LG. Thank you!

This revision was automatically updated to reflect the committed changes.

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