Page MenuHomePhabricator

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

Authored by ldionne on Jun 8 2020, 12:04 PM.



Fix LWG 2874.

Diff Detail

Event Timeline

zoecarver created this revision.Jun 8 2020, 12:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 12:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jun 9 2020, 8:05 AM

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?

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.


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.

This revision now requires changes to proceed.Jun 9 2020, 8:05 AM
zoecarver edited the summary of this revision. (Show Details)Jun 9 2020, 9:17 PM
zoecarver marked an inline comment as done.Jun 9 2020, 9:25 PM
zoecarver added inline comments.

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

zoecarver updated this revision to Diff 269730.Jun 9 2020, 9:28 PM
  • Update status.
  • Use TEST_STD_VER instead of the non-existant STD_TEST_VERSION.
  • Use std::conditional (instead of logical and/or).
zoecarver updated this revision to Diff 325083.Feb 19 2021, 1:50 PM
  • Rebase off main.
zoecarver updated this revision to Diff 325085.Feb 19 2021, 1:53 PM
  • Only use sfinae after C++03.
ldionne commandeered this revision.Apr 14 2021, 12:19 PM
ldionne edited reviewers, added: zoecarver; removed: ldionne.
ldionne updated this revision to Diff 337516.Apr 14 2021, 12:20 PM

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

zoecarver accepted this revision as: zoecarver.Apr 14 2021, 12:32 PM

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


Nit: spaces around =.


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:

  __compatible_with<_Yp, _Tp>::value 
  && (is_array<_Tp>::value ? 
        __is_array_deletable<_Yp*>::value : 
ldionne updated this revision to Diff 337522.Apr 14 2021, 12:39 PM
ldionne marked an inline comment as done.

Fix spacing.


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

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 accepted this revision as: Restricted Project.Apr 14 2021, 12:40 PM

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

This revision is now accepted and ready to land.Apr 14 2021, 12:40 PM
zoecarver added inline comments.Apr 14 2021, 12:42 PM

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 updated this revision to Diff 337788.Apr 15 2021, 9:21 AM

Try to fix the GCC build.