This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Fix LWG 2875: shared_ptr::shared_ptr(Y*, D, […]) constructors should be constrained.
ClosedPublic

Authored by zoecarver on Jun 8 2020, 11:21 AM.

Details

Summary

Fixes LWG issue 2875.

Diff Detail

Event Timeline

zoecarver created this revision.Jun 8 2020, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 11:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jun 9 2020, 8:13 AM

Do we have tests that shared_ptr isn't constructible from incompatible array types? If not, could we add some?

Otherwise, mostly LGTM.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
71

Nit: Please don't wrap here.

This revision now requires changes to proceed.Jun 9 2020, 8:13 AM
zoecarver updated this revision to Diff 269731.Jun 9 2020, 9:33 PM
  • Fix line wrapping

Do we have tests that shared_ptr isn't constructible from incompatible array types? If not, could we add some?

The __compatible_with trait removes the array extent. And (AFAIK) there are no array types that are convertible where their elements aren't. I'll make another patch to add tests that ensure we *can* construct a shared_ptr with array types that aren't convertible but where their elements are.

  • Rebase off main.
ldionne accepted this revision.Feb 17 2021, 2:55 PM

LGTM. I assume the array part of the requirements in https://wg21.link/LWG2875 are handled in your arrays-in-shared-ptr patch (which I need to get back to reviewing)?

Feel free to ship it after confirmation that my understanding above is right and after CI finishes. Thanks!

This revision is now accepted and ready to land.Feb 17 2021, 2:55 PM

LGTM. I assume the array part of the requirements in https://wg21.link/LWG2875 are handled in your arrays-in-shared-ptr patch (which I need to get back to reviewing)?

I don't think the wording changes anything about how arrays are handled. As I understand it the deleter requirements are the same for array types and non-array types. Also, the arrays-in-shared-ptr patch landed: D62259. A shared_ptr with array types still uses the same overload, that's what the __compatible_with trait is in charge of.

Sorry, I got confused with the patch for enabling arrays in make_shared and allocate_shared. For some reason I thought those two were more related than they actually are. Still LGTM.

  • Fix test failing for C++2b
zoecarver added inline comments.Feb 18 2021, 9:40 PM
libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
42 ↗(On Diff #324701)

I think I'm going to remove this test and replace it with a test that just checks we hit a static_assert when instantiating default_delete with a function type; no need to get shared_ptr involved.

zoecarver updated this revision to Diff 324880.Feb 18 2021, 9:50 PM
  • Remove shared_ptr from tests for default_delete.
zoecarver added inline comments.Feb 18 2021, 9:53 PM
libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
42 ↗(On Diff #324701)

Actually, never mind. I just removed shared_ptr from the part that was failing (because it added extra errors because a shared_ptr couldn't be constructed because it SFINAEed out). I think the other parts of this test are actually good to have. Anyway, hopefully that fixes the CI.