On Thu, Oct 29, 2020 at 2:12 PM Lénárd Szolnoki via Std-Proposals <
std-proposals_at_[hidden]> wrote:

>
> Calling `foo` is undefined behavior, since Base doesn't have a virtual
> destructor. No compiler seems to warn https://godbolt.org/z/xM53P5.
>

(Repeating a Slack comment)
Clang warns if either Base or Derived has at least one virtual function:
https://godbolt.org/z/17r3fs
This is not a "rebuttal" of your main point, because your non-virtual
case *still
has UB*, and Clang doesn't warn about it.
But you should mention/explain that Clang warns about the virtual case,
because half of your audience is going to dimly recollect that Clang does
warn here, and you can save them all some time and confusion.


> In fact these kind of misusage of unique_ptr could be detected and
> disallowed at the point of conversion, if std::default_delete didn't
> have a too generous converting constructor template.
>
> _Proposal_
>
> Limit std::default_delete's converting constructor template to only
> participate in overload resolution when it's safe to delete the
> resulting pointer.
>

(Repeating a Slack comment)
You should mention/explain that the existing standard *already constrains*
std::default_delete's converting constructor template. Right now, it is
constrained so that is_convertible<default_delete<T>, default_delete<U>> if
and only if is_convertible<T*, U*>.

I like simplicity, so I would tend to oppose adding constraints to
previously unconstrained templates. But, if you explain that the template
is already constrained *wrongly*, and your proposal is simply to *correct*
the buggy constraint, that changes my opinion from "weakly against" to
"strongly in favor."


Possible implementation:
>
> template <class T>
> struct default_delete {
> constexpr default_delete() noexcept = default;
>
> template <std::convertible_to<T> U>
>

This is not part of the correct constraint. You don't need U to be
convertible to T (e.g. T=int U=double); you need U* to be convertible to T*
(e.g. T=unique_ptr<int> U=unique_ptr<int>).


> requires (std::derived_from<U, T>
> && std::has_virtual_destructor_v<T>)
> || std::same_as<std::remove_cv_t<U>, std::remove_cv_t<T>>
> default_delete(const default_delete<U>&) noexcept {}
>

I would like to see this as a "Tony Table" ;) or a diff between what it is
today and what you are proposing. Right now, this code snippet looks
complicated. It should look simple, because it should be, like, inserting a
single additional line in the middle of an existing complicated SFINAE
condition.
(It's also not 100% obvious to me that this constructor can be constrained
with `requires`/concepts, versus SFINAE. It might be less nerdsnipey to use
regular SFINAE, since that's what vendors do today.)

Here's the diff for real libc++ (as I understand it). I'm not saying you
should actually include this in the paper, but I do think you should
include a simplified version of it.

*diff --git a/libcxx/include/memory b/libcxx/include/memory*

*index f3b0b04e80e..bce654724d6 100644*

*--- a/libcxx/include/memory*

*+++ b/libcxx/include/memory*

@@ -2134,8 +2134,11 @@ struct _LIBCPP_TEMPLATE_VIS default_delete {

   template <class _Up>

   _LIBCPP_INLINE_VISIBILITY

   default_delete(const default_delete<_Up>&,

- typename enable_if<is_convertible<_Up*,
_Tp*>::value>::type* =

- 0) _NOEXCEPT {}

+ typename enable_if<is_convertible<_Up*, _Tp*>::value>::type* = 0,

+ typename enable_if<

+ is_same<remove_cv<_Tp>::type, remove_cv<_Up>::type>::value ||

+ has_virtual_destructor<_Tp>::value>::type* = 0)

+ _NOEXCEPT {}



   _LIBCPP_INLINE_VISIBILITY void operator()(_Tp* __ptr) const _NOEXCEPT {

     static_assert(sizeof(_Tp) > 0,

I notice that all vendors' libraries currently permit the programmer to
construct instances of default_delete<void>, as long as they never call its
operator(). Do you think default_delete<int> *needs* to be convertible to
default_delete<void>? https://godbolt.org/z/x5Yd3E
If so, you'll have to carve out some more special cases.

Finally, can you mention and explain why default_delete<T[]>'s constructor
doesn't need any adjustment?

–Arthur