This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Don't generate incorrect fixes for class with deleted copy/move constructor in smart_ptr check.
ClosedPublic

Authored by hokein on Nov 20 2018, 2:06 AM.

Details

Summary

The fix for aggregate initialization (std::make_unique<Foo>(Foo {1, 2}) needs
to see Foo copy/move constructor, otherwise we will have a compiler error. So we
only emit the check warning.

Event Timeline

hokein created this revision.Nov 20 2018, 2:06 AM

Does make_unique require the copy constructor if it could move?
And would the same argument apply to the move-constructors as the arguments are forwarded?
What would happen in the obscure case of a public copy-constructor, but private move-constructor (not saying it makes sense :))

clang-tidy/modernize/MakeSmartPtrCheck.cpp
392

typo: s/consturctor/constructor/

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

please add a case for the private copy-constructor as well.

hokein updated this revision to Diff 174760.Nov 20 2018, 6:11 AM
hokein marked 2 inline comments as done.

Address review comments, handle move constructor as well.

Thanks for the review.

Does make_unique require the copy constructor if it could move?

No, in that case, move constructor will be used. I have updated the patch to include these cases.

And would the same argument apply to the move-constructors as the arguments are forwarded?
What would happen in the obscure case of a public copy-constructor, but private move-constructor (not saying it makes sense :))

This depends. The rule is complicated, for some cases it works, for other cases, it won't work.

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

Added more tests.

JonasToth added a project: Restricted Project.

IMHO this patch is fine, but i think a language expert (not me :D) should take a look (@aaron.ballman ?) as its complicated :)

hokein retitled this revision from [clang-tidy] Don't generate incorrect fixes for class with deleted copy constructor in smart_ptr check. to [clang-tidy] Don't generate incorrect fixes for class with deleted copy/move constructor in smart_ptr check..Nov 21 2018, 7:21 AM
hokein edited the summary of this revision. (Show Details)
aaron.ballman accepted this revision.Nov 23 2018, 7:14 AM

Generally LGTM with a few small nits.

clang-tidy/modernize/MakeSmartPtrCheck.cpp
391

requires to see -> needs to see

395

Don't use auto here.

396

How about:

if (llvm::find_if(RD->ctors(), [](const CXXConstructorDecl *CD) {
      return Ctor->isCopyOrMoveConstructor() &&
             (Ctor->isDeleted() || Ctor->getAccess() == AS_private);
    }))
  return false;
This revision is now accepted and ready to land.Nov 23 2018, 7:14 AM
hokein updated this revision to Diff 175205.Nov 26 2018, 1:26 AM
hokein marked 3 inline comments as done.

Address review comments.

This revision was automatically updated to reflect the committed changes.