On Sat, Jul 18, 2020 at 11:10 AM Jason McKesson via Std-Proposals <
std-proposals_at_[hidden]> wrote:

> On the technical side of things, there are a few more issues.
>
> The type should not be copyable. Creating non-owning references is
> something that can lead to bugs, so it shouldn't be able to happen by
> accident. [...]
> To avoid this, the type ought not have a copy constructor/assignment
> operator. Instead, it should have a `nonowning` member function that
> returns a prvalue "copy" of the object that doesn't own the pointer.
> So for the above example, we have `func(a_ptr.nonowning())`. This
> makes it very obvious what is going on, and you can look for places in
> your code where you're calling that function.
>

Naming bikeshed: `.nonowning()` is a bad name because it seems to analogize
with `.empty()` as a query — "is this smart-pointer non-owning?" — rather
than as a getter. One general pattern for fixing this kind of name is to
prepend ".as_": "a_ptr.as_nonowning()".
However, in this specific case, the STL's smart pointers already have a
short idiomatic name for "please give me a non-owning copy of this
smart-pointer": it's spelled `a_ptr.get()`! Of course for the standard
smart pointers, the only way to get a non-owning pointer is to return a
different type, so `.get()` returns `T*`. I wouldn't suggest ever returning
something other than `T*` from `.get()`.
*But...*
As originally presented, the type had an implicit constructor from `T*`
that produced a *non-owning* smart-pointer. That is, it violated the STL's
patterns again in two different ways: implicit conversion from raw to
smart, and conversion from raw to *smart non-owning* instead of to *smart
owning*.
These three pattern-violations make this a bad candidate for adding to the
STL, but they do work together to create an idiom that might be acceptable
if the STL didn't already exist. Consider:

    template<class T>
    struct context_dependent_ptr {
        T *p_;
        bool owns_ = false;
        context_dependent_ptr(T *p) : p_(p) {}
        context_dependent_ptr(const context_dependent_ptr&) = delete;
        context_dependent_ptr(context_dependent_ptr&& rhs) : p_(rhs.p_),
owns_(std::exchange(rhs.owns_, false)) {}
        static context_dependent_ptr adopt(T *p) { context_dependent_ptr
cdp(p); cdp.owns_ = true; return cdp; }
        auto& operator=(context_dependent_ptr rhs) { this->swap(rhs);
return *this; }
        void swap(context_dependent_ptr& rhs) { std::swap(p_, rhs.p_);
std::swap(owns_, rhs.owns_); }
        T *get() const { return p_; }
        ~context_dependent_ptr() { if (owns_) delete p_; }
    };

    void func(context_dependent_ptr<int> param);

    int local = 42;
    std::unique_ptr<int> uptr = std::make_unique<int>(42);
    context_dependent_ptr<int> cd1 = context_dependent_ptr<int>::adopt(new
int(42));
    context_dependent_ptr<int> cd2 = &local;

    func(&local); // param is non-owning
    func(uptr); // does not compile
    func(cd1); // does not compile
    func(cd2); // does not compile
    func(uptr.get()); // param is non-owning
    func(cd1.get()); // param is non-owning
    func(cd2.get()); // param is non-owning
    func(std::move(uptr)); // does not compile
    func(std::move(cd1)); // param is owning
    func(std::move(cd2)); // param is non-owning

Notice that if move is going to do something significantly different from
copy (as in this case), then one or the other must be deleted.
This is really just another application of Titus Winters' mantra that
"overload sets should consist only of functions that *do the same thing*."
Copy and move are in the same overload set, so they should do the same
thing. "Taking ownership" versus "not taking ownership" are two *very*
different things.

    context_dependent_ptr<Derived> d
= context_dependent_ptr<Derived>::adopt(new Derived);
    context_dependent_ptr<Base> b = std::move(d);

What happens here, and why? Well, it depends on whether there exists an
rvalue-enabled constructor
context_dependent_ptr<Base>::context_dependent_ptr(context_dependent_ptr<Derived>&&).
If it exists, then this takes ownership. If it doesn't exist, then this
falls back onto context_dependent_ptr<Base>::context_dependent_ptr(const
context_dependent_ptr<Base>&) and we get a copy (which doesn't take
ownership).

shared_ptr and weak_ptr have a similar issue — you can't "move" a
shared_ptr into a weak_ptr — but there it's okay, because copy and move do
basically the same thing from the programmer's point of view. (Leaving an
extra refcount hanging around in the source object is suboptimal but not
terrible, whereas *failing to take ownership* would be terrible.)

Since the outcome of picking the wrong overload here would be terrible,
it's important to put these two vastly different functionalities under
*different
names*. Don't call them "(copy) construct" and "(move) construct." Calling
them "get" and "(move) construct" seems to fix the problem in a clever way.
I wouldn't bet money that there isn't some other subtle problem with doing
it that way, though. This whole type still seems extremely unsafe to me and
I would never use it in production code.

–Arthur