Fix LWG 2874.
- Group Reviewers
- rG1f8a6dcf1280: [libc++] Fix LWG 2874: Constructor shared_ptr::shared_ptr(Y*) should be…
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.
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:
_EnableIf< __compatible_with<_Yp, _Tp>::value #if && (is_array<_Tp>::value ? __is_array_deletable<_Yp*>::value : __is_deletable<_Yp*>::value) #endif >
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.
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.