[libcxx] Fix LWG 2874: Constructor shared_ptr::shared_ptr(Y*) should be constrained.

[ldionne]

Summary

Fix LWG 2874.

[ldionne]

Nitpick: You've got the wrong LWG number in your commit message.

Also, can you please mark the LWG issue as complete in the docs?

libcxx/include/memory

3565 ↗

(On Diff #269307)

You are eagerly instantiating all templates here, and I don't think that's what you want. For example, you'll instantiate __well_formed_delete_op<_Yp*> even when is_array<_Tp>::value is true.

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

122

STD_TEST_VERSION doesn't exist, it's TEST_STD_VER.

To catch these sorts of errors, it can be good to add tests and witness their failure before actually fixing them.

[zoecarver]

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

122

Oops, you're right. Somehow that got suck in my clipboard history.

[zoecarver]

• Update status.
• Use TEST_STD_VER instead of the non-existant STD_TEST_VERSION.
• Use std::conditional (instead of logical and/or).

[zoecarver]

• Rebase off main.

[zoecarver]

• Only use sfinae after C++03.

[ldionne]

Rebase onto <memory> changes and refactor the patch for clarity.

[zoecarver]

Two nits, feel free to take or leave them. LGTM.

libcxx/include/__memory/shared_ptr.h

382

Nit: spaces around =.

439

Another nit, feel free to take it or leave it: I don't think we need to introduce _And and _If here. Why don't we just do:

_EnableIf<
  __compatible_with<_Yp, _Tp>::value 
#if
  && (is_array<_Tp>::value ? 
        __is_array_deletable<_Yp*>::value : 
        __is_deletable<_Yp*>::value)
#endif
>

[ldionne]

Fix spacing.

libcxx/include/__memory/shared_ptr.h

439

The problem here is that we only want to instantiate __is_array_deletable<_Yp*> or __is_deletable<_Yp*> when is_array<_Tp> is true/false, respectively. With your approach, we do it eagerly.

Similarly, we could do this:

__compatible_with<_Yp, _Tp>::value
#ifndef _LIBCPP_CXX03_LANG
  && _If<is_array<_Tp>::value, __is_array_deletable<_Yp*>, __is_deletable<_Yp*> >::value
#endif

But then, if the types are not compatible, we'll still be instantiating one check for __is_array_deletable or __is_deletable, depending on whether the type is an array.

The current approach delays and short cirtcuits the instantiation of SFINAE helpers as long as is possible. The only thing we do check eagerly is is_array<_Tp> regardless of the result of __compatible_with.

Furthermore, it used to be the case that we'd want to minimize these sorts of control structures in TMP code because they were heavier. However, nowadays we're using aliases to implement them, and some folks like Odin Holmes have demonstrated that they are very cheap.

[ldionne]

Will ship once CI is finished (assuming it's green).

[zoecarver]

libcxx/include/__memory/shared_ptr.h

439

Fair enough. I'm convinced :)

And I suppose you're right, the SFINAE type traits are going to be more expensive than _And and _If, so it makes sense to eagerly instantiate only the latter, even if it means slightly more code.

[ldionne]

Try to fix the GCC build.