Implementation of std::experimental::observer_ptr (library fundamentals TS v2)

Question

I made an implementation of std::experimental::observer_ptr (library fundamentals TS v2). observer_ptr basically behaves like a normal pointer and does no management of its target whatsoever (unlike std::unique_ptr or std::shared_ptr). It's usage is solely to indicate that it takes no ownership of the target object (it only observes it).

The implementation is simple and straight forward.

#include <cstddef>
#include <type_traits>
#include <memory>


namespace tb
{
    template <typename T>
    class observer_ptr
    {
    public:
        using element_type = T;

        constexpr observer_ptr() noexcept = default;
        constexpr observer_ptr(std::nullptr_t) noexcept { }

        template <typename U, typename = std::enable_if<!std::is_same_v<element_type, U> && std::is_convertible_v<U*, element_type*>>>
        observer_ptr(observer_ptr<U> const& other) :
            observer_ptr(static_cast<element_type*>(other.get())) {}

        explicit observer_ptr(element_type* ptr) :
            _data(ptr) { }

        constexpr element_type* release() noexcept
        {
            auto* ptr = _data;
            _data = nullptr;
            return ptr;
        }

        constexpr void reset(element_type* p = nullptr) noexcept { _data = p; }

        constexpr void swap(observer_ptr& other) noexcept
        {
            using std::swap;
            swap(_data, other._data);
        }

        constexpr friend void swap(observer_ptr& lhs, observer_ptr& rhs) noexcept { lhs.swap(rhs); }

        [[nodiscard]] constexpr element_type* get() const noexcept { return _data; }

        [[nodiscard]] constexpr std::add_lvalue_reference_t<element_type> operator*() const { return *get(); }

        [[nodiscard]] constexpr element_type* operator->() const noexcept { return get(); }

        [[nodiscard]] constexpr explicit operator bool() const noexcept { return _data == nullptr; }

        [[nodiscard]] constexpr explicit operator element_type*() const noexcept { return get(); }

    private:
        element_type* _data = nullptr;
    };

    template <typename T>
    [[nodiscard]] observer_ptr<T> make_observer(T* ptr) noexcept
    {
        return observer_ptr<T>(ptr);
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator==(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return p1.get() == p2.get();
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator!=(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return !(p1 == p2);
    }

    template <typename T>
    [[nodiscard]] bool operator==(observer_ptr<T> const& p, std::nullptr_t) noexcept
    {
        return static_cast<bool>(p);
    }

    template <typename T>
    [[nodiscard]] bool operator==(std::nullptr_t, observer_ptr<T> const& p) noexcept
    {
        return static_cast<bool>(p);
    }

    template <typename T>
    [[nodiscard]] bool operator!=(observer_ptr<T> const& p, std::nullptr_t) noexcept
    {
        return !p;
    }

    template <typename T>
    [[nodiscard]] bool operator!=(std::nullptr_t, observer_ptr<T> const& p) noexcept
    {
        return !p;
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator<(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return p1.get() < p2.get();
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator>(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return p2 < p1;
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator<=(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return !(p2 < p1);
    }

    template <typename T1, typename T2>
    [[nodiscard]] bool operator>=(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
    {
        return !(p1 < p2);
    }
}

Edit

Here is a sample usage

#include <vector>
#include <iostream>

struct A 
{
    void hello() const { std::cout << "hello\n"; }    
};

// indicates that foo won't take ownership
void foo(tb::observer_ptr<A> const& ptr) { ptr->hello(); }

// does foo take ownership of ptr (especially if it were a member function)?
void foo(A* ptr) { ptr->hello(); }

int main() 
{
    std::vector<std::unique_ptr<A>> manager;
    manager.emplace_back(new A);
    manager.emplace_back(new A);
    manager.emplace_back(new A);

    for (auto& a : manager)
    {
        foo(tb::observer_ptr(a.get()));
        foo(a.get());
    } 

    return 0;
}

Some question that came to my mind:

1. Should I add constructor overloads for std::unique_ptr and std::shared_ptr (for convenience)?
2. I'm usually not a fan of implicit conversions but in this case I'm thinking about making the constructors implicit. This is justified by (a) observer_ptr is a fairly light weight type and (b) it doesn't change the behavior or affect the underlying pointer in any way. What do you think about that?

Answer 1

Some suggestions:

1. Don't prefix the data member with an underscore. The rule about identifiers starting with an underscore are easy to get wrong. In this case, just name it data.

2. You did not make the converting constructor and the raw pointer constructor noexcept. They are noexcept according to the spec.

3. Don't use element_type everywhere, it's too long. Just use T.

4. Don't breach the 80 character limit. Scrolling is a bit annoying.

   template <typename U, typename = std::enable_if<!std::is_same_v<element_type, U> && std::is_convertible_v<U*, element_type*>>>
   observer_ptr(observer_ptr<U> const& other)
   

   The long line can be broken down. typename = std::enable_if is useless — you probably meant typename = std::enable_if_t. And the !std::is_same_v<T, U> is redundant because the copy constructor will always take precedence over this constructor:

   template <typename U, typename = std::enable_if_t<std::is_convertible_v<U*, T*>>>
   observer_ptr(obverser_ptr<U> other) noexcept
       :data{other.get()}
   {
   }
   

5. The release function can be simplified with std::exchange:

   return std::exchange(data, nullptr);
   

6. The operator bool is getting the logic wrong — it should return data != nullptr instead.

7. The lines are getting a bit long for the observers. I prefer writing them on separate lines:

   [[nodiscard]] constexpr std::add_lvalue_reference_t<T> operator*() const
   {
       return *get();
   }
   [[nodiscard]] constexpr T* operator->() const noexcept
   {
       return get();
   }
   

8. According to the spec, operator< should use std::less of the composite pointer type instead of the builtin < on pointers because the latter does not provide a strict total order. So:

   template <typename T1, typename T2>
   [[nodiscard]] bool operator<(observer_ptr<T1> const& p1, observer_ptr<T2> const& p2)
   {
       using CP = /* work out the composite pointer type of T1* and T2* */;
       return std::less<CP>;
   }
   

9. Where's std::hash? You need to specialize it.