This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Allow shared_ptr's unique_ptr converting constructor to support array types.
ClosedPublic

Authored by zoecarver on May 30 2020, 3:11 PM.

Diff Detail

Event Timeline

zoecarver created this revision.May 30 2020, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2020, 3:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Friendly ping.

zoecarver updated this revision to Diff 325087.Feb 19 2021, 2:16 PM
  • Add test for lvalue deleter.
  • Format with clang-format.
ldionne requested changes to this revision.Apr 8 2021, 8:47 AM

Sorry this fell through the cracks. Let's fix this!

Do we have the same problem for operator=? We should rebase and make sure CI passes - LMK if you'd like me to commandeer this.

This revision now requires changes to proceed.Apr 8 2021, 8:47 AM
zoecarver updated this revision to Diff 336176.Apr 8 2021, 11:15 AM
  • Remove constraint not is_array from overloads.
zoecarver added inline comments.Apr 8 2021, 11:16 AM
libcxx/include/memory
3257

Interestingly enough, updating the enable_if here wasn't required to get the tests to pass, I think because of implicit conversion to shared_ptr before the assignment happens.

ldionne added inline comments.Apr 8 2021, 11:45 AM
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.assign/unique_ptr_Y.pass.cpp
131 ↗(On Diff #336176)

This is not testing operator=, this is testing initialization!

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp
13

Hmm, this shouldn't be explicit. It looks like the implementation is OK, only this comment is wrong. Can you fix as a fly-by?

zoecarver updated this revision to Diff 336184.Apr 8 2021, 11:56 AM
  • Test the right thing!
  • Remove "explicit" in ctor comment.
ldionne accepted this revision.Apr 8 2021, 11:57 AM

Thanks! Ship it once CI passes. And you can then close https://llvm.org/PR32147. Thanks a lot!

This revision is now accepted and ready to land.Apr 8 2021, 11:57 AM

For sure, will do, thanks for reviewing!

zoecarver added inline comments.Apr 8 2021, 12:52 PM
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/unique_ptr.pass.cpp
181

shared_ptr<array[]> wasn't supported until C++17, so these tests shouldn't be enabled until C++17. That's why the C++14 build is failing.

zoecarver updated this revision to Diff 336200.Apr 8 2021, 12:53 PM
  • Enable array->array tests after C++14 only
ldionne accepted this revision.Apr 8 2021, 12:59 PM

Still LGTM if CI passes. Thanks!