A moveable QScopedPointer, or a Qt and std cross-compatible unique_pointer

Question

I'm a hobby programmer, so I've never been through a code review before (online or offline). That said, here goes:

Background

I use Qt extensively but have long wanted QScopedPointer to be a moveable type. Since the Qt devs don't plan to implement this, I started trying to implement it myself. This was several years ago. [Pause for laughter]

Purpose

My goal is to not simply add move semantics to QScopedPointer:

• To maintain as much compatibility with the Standard Library as reasonably possible by
  • adding a partial specialization of std::hash and std::swap,
  • declaring the expected type traits,
  • and implementing all of the public member functions in std::unique_pointer.
• To also maintain compatibility with Qt, so that qe::UniquePointer acts as a drop-in replacement for QScopedPointer.
• Use C++11 and later where appropriate (e.g., explicit operator bool). Currently, it compiles with C++14 as a minimum.

Notes

I kept the Qt-style deleter to allow use of existing deleters for QScopedPointer. The lambdas and macros in test.h are my quick-and-dirty GTest stand-in.

Goals

• I want to make sure the fundamentals are sound. Despite testing, experienced eyeballs are good.
• I don't use Boost and don't have a lot of Standard Library experience. Did I miss any using declarations? Is my partial specialization of std::hash correctly done?
• I'm least confident about the conversion functions (accepting qe::UniquePointer<U, Cleanup>). Have I made any obvious mistakes that may create edge cases?
• Of course, I may have missed something for which the test class simply does not test. Any insight there would be appreciated.

Code

uniquepointer.h

#ifndef QE_CORE_UNIQUEPOINTER_H
#define QE_CORE_UNIQUEPOINTER_H

#include <type_traits>

namespace qe {
namespace detail {

template <class T>
constexpr void assertCompleteType() noexcept
{
    using forced_complete_type = char[sizeof(T) ? 1 : -1];
    (void)sizeof(forced_complete_type);
}

template <class T, class U>
void assertConvertible()
{
    using convertible = char[sizeof(std::is_convertible<T, U>()) ? 1 : -1];
    (void)sizeof(convertible);
}

} // namespace detail

template <typename T>
struct DefaultDeleter
{
    static inline void cleanup(T *pointer)
    {
        if (pointer) {
            qe::detail::assertCompleteType<T>();
            delete pointer;
        }
    }
};

template <class T, class Cleanup = DefaultDeleter<T>>
class UniquePointer
{
public:
    using this_type = UniquePointer<T, Cleanup>;
    using element_type = T;
    using pointer = std::add_pointer_t<T>;
    using const_pointer = std::add_const_t<pointer>;
    using reference = std::add_lvalue_reference_t<T>;
    using const_reference = std::add_const_t<reference>;
    using deleter_type = Cleanup;

    inline UniquePointer(pointer p = nullptr) noexcept : d(p) {}
    inline UniquePointer(this_type && other) noexcept : d(other.take()) { }

    template <class U>
    inline UniquePointer(UniquePointer<U, Cleanup> && other) noexcept(std::is_convertible<U, T>())
        : d(other.take())
    {
        qe::detail::assertConvertible<U, T>();
    }

    inline this_type &operator=(this_type &&other)
    {
        if (*this != other)
            reset(other.take());
        return *this;
    }

    template <class U>
    inline this_type &operator=(UniquePointer<U, Cleanup> &&other)
        noexcept(std::is_convertible<U, T>())
    {
        qe::detail::assertConvertible<U, T>();
        if (*this != other)
            reset(other.take());
        return *this;
    }

    UniquePointer(const this_type &) = delete;
    this_type &operator=(const this_type &) = delete;

    inline ~UniquePointer()
    {
        reset(nullptr);
    }

    pointer take() noexcept
    {
        auto oldD = this->d;
        this->d = nullptr;
        return oldD;
    }

    inline void reset(pointer other = nullptr)
    {
        if (this->d == other)
            return;
        auto oldD = this->d;
        if (oldD)
            deleter_type::cleanup(oldD);
        this->d = other;

    }

    inline pointer data() const noexcept            { return this->d; }
    inline pointer get() const noexcept             { return this->d; }
    inline pointer* addressOf() noexcept            { return &(this->d); }
    explicit operator bool() const noexcept         { return !isNull(); }
    inline bool operator!() const noexcept          { return isNull(); }
    inline reference operator*() const noexcept     { return *(this->d); }
    inline pointer operator->() const noexcept      { return this->d; }
    inline bool isNull() const noexcept             { return !(this->d); }
    inline void swap(UniquePointer &other) noexcept { std::swap(this->d, other.d); }

    template <class U>
    void swap(UniquePointer<U, Cleanup> &other)
        noexcept(std::is_convertible<U, T>() && std::is_convertible<T, U>())
    {
        qe::detail::assertConvertible<U, T>();
        qe::detail::assertConvertible<T, U>();
        std::swap(this->d, other.d);
    }

    pointer release() noexcept              { return take(); }

private:
    pointer d;
};

template <class T, class... Args>
inline UniquePointer<T> makeUnique(Args && ...args)
{
    return UniquePointer<T>(new T(std::forward<Args>(args)...));
}

} //namespace qe

template <class T, class Cleanup>
inline bool operator==(const qe::UniquePointer<T, Cleanup> &lhs, const qe::UniquePointer<T, Cleanup> &rhs) noexcept
{
    return lhs.data() == rhs.data();
}

template<class T, class Cleanup>
inline bool operator ==(const qe::UniquePointer<T, Cleanup> &lhs, std::nullptr_t) noexcept
{
    return lhs.isNull();
}

template<class T, class Cleanup>
inline bool operator ==(std::nullptr_t, const qe::UniquePointer<T, Cleanup> &rhs) noexcept
{
    return rhs.isNull();
}

template <class T, class Cleanup>
inline bool operator!=(const qe::UniquePointer<T, Cleanup> &lhs, const qe::UniquePointer<T, Cleanup> &rhs) noexcept
{
    return lhs.data() != rhs.data();
}

template <class T, class Cleanup>
inline bool operator !=(const qe::UniquePointer<T, Cleanup> &lhs, std::nullptr_t) noexcept
{
    return !lhs.data();
}

template <class T, class Cleanup>
inline bool operator !=(std::nullptr_t, const qe::UniquePointer<T, Cleanup> &rhs) noexcept
{
    return !rhs.data();
}

namespace std {
template <class T, class Cleanup>
inline void swap(qe::UniquePointer<T, Cleanup> &lhs, qe::UniquePointer<T, Cleanup> &rhs) noexcept
{
    lhs.swap(rhs);
}

template <class T, class Cleanup>
struct hash<qe::UniquePointer<T, Cleanup>>
{
    using argument_type = qe::UniquePointer<T, Cleanup>;
    using result_type = std::size_t;
    inline result_type operator()(const argument_type & p) const noexcept
    {
        return std::hash<T *>{}(p.data());
    }
};
} //namespace std

#endif //QE_CORE_UNIQUEPOINTER_H

test.h

#ifndef QE_TEST_TEST_H
#define QE_TEST_TEST_H

#include <assert.h>

auto equal_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs == rhs) && (rhs == lhs);
};

auto not_equal_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs != rhs);
};

auto less_than_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs < rhs);
};

auto less_than_or_equal_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs <= rhs);
};

auto greater_than_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs > rhs);
};

auto greater_than_or_equal_check = [](auto && lhs, auto && rhs) -> bool
{
    return (lhs >= rhs);
};

auto implicit_true_check = [](auto && lhs) -> bool
{
    return lhs ? true : false;
};

auto implicit_false_check = [](auto && lhs) -> bool
{
    return lhs ? false : true;
};

#define EXPECT_EQ(x, y) assert(equal_check(x, y))
#define EXPECT_NE(x, y) assert(not_equal_check(x, y))
#define EXPECT_LT(x, y) assert(less_than_check(x, y))
#define EXPECT_LE(x, y) assert(less_than_or_equal_check(x, y))
#define EXPECT_GT(x, y) assert(greater_than_check(x, y))
#define EXPECT_GE(x, y) assert(greater_than_or_equal_check(x, y))
#define EXPECT_TRUE(x)  assert(implicit_true_check(x))
#define EXPECT_FALSE(x) assert(implicit_false_check(x))

#endif // QE_TEST_TEST_H

main.cpp

#include <vector>
#include "uniquepointer.h"
#include "test.h"

using namespace qe;

struct Struct1
{
    explicit Struct1(int aVal)
        : value(aVal)
    {
        instances++;
    }

    Struct1(const Struct1 &) = default;
    Struct1(Struct1 &&) = default;

    ~Struct1()
    {
        instances--;
    }

    Struct1 &operator =(const Struct1 &) = default;
    Struct1 &operator =(Struct1 &&) = default;

    void incr()
    {
        value++;
    }

    void decr()
    {
        value--;
    }

    int value = 0;
    static int instances;
};

int Struct1::instances = 0;

struct Struct2 : public Struct1
{
    explicit Struct2(const int aVal)
        : Struct1(aVal)
    {

    }
};


struct Struct3 : public Struct1
{
    Struct3(const Struct2 &other)
        : Struct1(other.value)
    {
    }
    Struct3(const Struct2 &&other)
        : Struct1(other.value)
    {
    }
    Struct3 operator=(const Struct2 &other)
    {
        Struct3 ret(other);
        return ret;

    }

    Struct3 operator=(Struct2 &&other)
    {
        Struct3 ret(other);
        other.~Struct2();
        return ret;
    }
};

struct unique_pointer_test
{

    static void run()
    {
        empty_pointer_test();
        basic_pointer_test();
        reset_pointer_test();
        compare_pointer_test();
        swap_pointer_test();
        std_container_test();
    }

    static void empty_pointer_test()
    {
        // Create an empty (ie. nullptr) UniquePointer
        UniquePointer<Struct2> xPtr = nullptr;

        EXPECT_FALSE(xPtr);
        EXPECT_EQ(nullptr, xPtr.get());

        // Reset to nullptr (ie. do nothing)
        xPtr.reset();

        EXPECT_FALSE(xPtr);
        EXPECT_EQ(nullptr,  xPtr.get());
    }

    static void basic_pointer_test()
    {
        {
            // Create a UniquePointer
            UniquePointer<Struct2> xPtr(new Struct2(123));

            EXPECT_TRUE(xPtr);
            EXPECT_NE(nullptr, xPtr.get());

            if (xPtr)
            {
                EXPECT_EQ(123,  xPtr->value);
                EXPECT_EQ(1,    xPtr->instances);
                EXPECT_EQ(1,    Struct1::instances);

                // call a function
                xPtr->incr();
                EXPECT_EQ(124,  xPtr->value);
                (*xPtr).incr();
                EXPECT_EQ(125,  (*xPtr).value);
                xPtr->decr();
                xPtr->decr();

                // Copy construct the UniquePointer, transferring ownership
                UniquePointer<Struct2> yPtr(std::move(xPtr));
                xPtr.reset();

                EXPECT_NE(xPtr, yPtr);
                EXPECT_FALSE(xPtr);
                EXPECT_EQ(nullptr,  xPtr.get());
                EXPECT_TRUE(yPtr);
                EXPECT_NE(nullptr,  yPtr.get());
                EXPECT_EQ(123,   yPtr->value);
                EXPECT_EQ(1,     Struct1::instances);

                if (yPtr)
                {
                    UniquePointer<Struct2> zPtr = std::move(yPtr);
                    yPtr.reset();

                    EXPECT_NE(yPtr,  zPtr);
                    EXPECT_FALSE(yPtr);
                    EXPECT_EQ(nullptr, yPtr.get());
                    EXPECT_TRUE(zPtr);
                    EXPECT_NE(nullptr,  zPtr.get());
                    EXPECT_EQ(123,   zPtr->value);
                    EXPECT_EQ(1,     Struct1::instances);
                }

                EXPECT_FALSE(xPtr);
                EXPECT_EQ(nullptr, xPtr.get());
                EXPECT_FALSE(yPtr);
                EXPECT_EQ(nullptr, yPtr.get());
                EXPECT_EQ(0, Struct1::instances);
            }
            else
            {
                assert(false); //"bool cast operator error"
            }

            EXPECT_FALSE(xPtr);
            EXPECT_EQ(nullptr, xPtr.get());
            EXPECT_EQ(0, Struct1::instances);
        }

        EXPECT_EQ(0, Struct1::instances);
    }

    static void reset_pointer_test()
    {
        // Create an empty (ie. nullptr) UniquePointer
        UniquePointer<Struct2> xPtr;

        // Reset it with a new pointer
        xPtr.reset(new Struct2(123));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(123,  xPtr->value);
        EXPECT_EQ(1,    Struct1::instances);
        Struct2* pX  = xPtr.get();

        // Reset it with another new pointer
        xPtr.reset(new Struct2(234));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(234,  xPtr->value);
        EXPECT_EQ(1,    Struct1::instances);
        EXPECT_NE(pX,   xPtr.get());

        // Move-construct a new UniquePointer to the same object, transferring ownership
        UniquePointer<Struct2> yPtr = std::move(xPtr);
        xPtr.reset();

        EXPECT_NE(xPtr, yPtr);
        EXPECT_FALSE( xPtr);
        EXPECT_EQ(nullptr,  xPtr.get());
        EXPECT_TRUE( yPtr);
        EXPECT_NE(nullptr,  yPtr.get());
        EXPECT_EQ(234,   yPtr->value);
        EXPECT_EQ(1,     Struct1::instances);

        // Reset to nullptr
        yPtr.reset();

        EXPECT_EQ(nullptr,  yPtr.get());
        EXPECT_FALSE(xPtr);
        EXPECT_EQ(nullptr,  xPtr.get());
        EXPECT_EQ(0, Struct1::instances);
    }

    static void compare_pointer_test()
    {
        // Create a UniquePointer
        UniquePointer<Struct2> xPtr(new Struct2(123));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(123,xPtr->value);
        EXPECT_EQ(1, Struct1::instances);
        Struct2* pX = xPtr.get();

        // Create another UniquePointer
        UniquePointer<Struct2> yPtr(new Struct2(234));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(123,xPtr->value);
        EXPECT_EQ(2, Struct1::instances);

        EXPECT_TRUE(yPtr);
        EXPECT_NE(nullptr, yPtr.get());
        EXPECT_EQ(234, yPtr->value);
        Struct2* pY = yPtr.get();


        EXPECT_NE(xPtr, yPtr);
        EXPECT_NE(pY->value, pX->value);
    }

    static void swap_pointer_test()
    {
        // Create a UniquePointer
        UniquePointer<Struct2> xPtr(new Struct2(123));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(123,xPtr->value);
        EXPECT_EQ(1, Struct1::instances);

        // Create another UniquePointer
        UniquePointer<Struct2> yPtr(new Struct2(234));

        EXPECT_TRUE(yPtr);
        EXPECT_NE(nullptr, yPtr.get());
        EXPECT_EQ(234, yPtr->value);
        EXPECT_EQ(2, Struct1::instances);

        EXPECT_LT(xPtr->value, yPtr->value);
        xPtr.swap(yPtr);
        EXPECT_GT(xPtr->value, yPtr->value);
        EXPECT_TRUE(xPtr);
        EXPECT_TRUE(yPtr);
    }

    static void std_container_test()
    {
        // Create a shared_ptr
        UniquePointer<Struct2> xPtr(new Struct2(123));

        EXPECT_TRUE(xPtr);
        EXPECT_NE(nullptr, xPtr.get());
        EXPECT_EQ(123, xPtr->value);
        EXPECT_EQ(1, Struct1::instances);
        Struct2* pX = xPtr.get();

        {
            std::vector<UniquePointer<Struct2> > PtrList;

            // Move-it inside a container, transferring ownership
            PtrList.push_back(std::move(xPtr));

            EXPECT_FALSE(xPtr);
            EXPECT_TRUE( PtrList.back());
            EXPECT_EQ(pX,PtrList.back().get());
            EXPECT_EQ(1, Struct1::instances);

        } // Destructor of the vector releases the last pointer thus destroying the object

        EXPECT_EQ(0, Struct1::instances);
    }

};

int main(int argc, char *argv[])
{
    (void)(argc);
    (void)(argv);

    unique_pointer_test::run();

    return 0;
}

Edit: here's the code on Wandbox

Edit 2: Removed need for C++17. Compiles with C++14.

Answer 1

Well, how about a radical idea:

Just use std::unique_ptr, the only extra-capability yours has is .addressOf(). Yes, you might have to write replacements for QScopedPointerPodDeleter and QScopedPointerDeleteLater if you use either, but that isn't hard:

struct FreeDeleter {
    static void operator()(void* p) noexcept { free(p); }
};
struct DeleteLater {
    template <class T>
    static void operator()(T* p) noexcept { p->deleteLater(); }
};

Anyway, now let's criticise how you wrote your own one:

1. static_assert is the tool of choice for causing compile-time-errors if something doesn't check out at compile-time. Learn to use and love it.

2. You can safely delete or free a null-pointer. It's defined to do nothing.

3. Inside the class-definition and out-of-line member-function-definition-bodies, you can use the injected name UniquePointer to refer to refer to the class. No need to mention the template-parameters unless you want some unrelated class from the same template.

4. Decorating an in-class-definition with inline is absolutely pointless.

5. I'm not sure why you put that expression into the noexcept-specification of the templated ctor. You should either use it for SFINAE, or a static_assert, but this makes no sense at all. I prefer the first:

   template <class U, typename std::enable_if<std::is_convertible<U, T>()>::type>
   UniquePointer(UniquePointer<U, Cleanup> && other) noexcept
   : d(other.take()) {}
   

6. I suggest doing minimal work when move-assigning: Simply swap.

7. You know, simply delegating to what you already have written would simplify your templated move-assignment-operator:

   template <class U>
   auto operator=(UniquePointer<U, Cleanup>&& other) noexcept
   -> decltype((*this = UniquePointer(other))) // The extra-parens are critical
   { return (*this = UniquePointer(other)); }
   

8. Copy-ctor and copy-assignment-operator are already implicitly deleted.

9. I wonder why you don't take advantage of the default-argument to .reset() in the dtor.

10. There is something really disturbing going on when you try to make your smart-pointer responsible for what it already is responsible for. It might make sense to assert that doesn't happen, but are you really sure that should silently do nothing?

11. Also in .reset(), I got to wondering whether the deleter is responsible for handling null-pointers, or your smart-pointer, or both? You seem to really like testing for null-pointers everywhere, anyway the compiler should be able to deduplicate the code just fine.

12. Your templated .swap() is utterly useless. Again, the expression which should be used for SFINAE is in the noexcept-specification, and you force a compile-time-error later when the body gets compiled instead, unless U is T. In that case though, the non-templated version claims precedence.

13. Write operator== and operator!= differently, and they will be far more comprehensive and useful:

    template <class T, class D, class U>
    constexpr auto operator==(const UniquePointer<T, D>& a, const U& b) noexcept
    -> decltype(b == a.d)
    { return b == a.d; }
    template <class T, class D, class U>
    constexpr auto operator==(const U& a, const UniquePointer<T, D>& b) noexcept
    -> std::enable_if_t<std::is_pointer<U>() || std::is_null_pointer<U>(), decltype(a == b.d)>
    { return a == b.d; }
    

    That also covers comparing with raw pointers, possible down-casting, and smart-pointers following the same pattern.

14. You should not put your own swap() in ::std. Even though it probably works, it's not allowed. Either rely on ADL to pick it up from the same namespace as your type, or depend on std::swap() (which should be good enough anyway).

Answer 2

Congrats, and good luck with your endeavors.

---

template <class T>
constexpr void assertCompleteType() noexcept
{
    using forced_complete_type = char[sizeof(T) ? 1 : -1];
    (void)sizeof(forced_complete_type);
}

Huh? A constexpr that doesn’t return a value, and an expression that does nothing?

I think you want to use static_assert, not a trick to cause an error in template instantiation.

static_assert (sizeof(T) > 0);
static_assert (std::is_convertible_v<T,U>);

---

inline UniquePointer(pointer p = nullptr) noexcept : d(p) {}
inline UniquePointer(this_type && other) noexcept : d(other.take()) { }

You don’t have to use inline when putting the body of the function inside the class.

Use the usual names for things: take should be release

Use =delete on the copy constructor and assignment operator, to prevent them from being automatically generated. Do provide a move assignment operator.

---

template <class U>
inline this_type &operator=(UniquePointer<U, Cleanup> &&other)
    noexcept(std::is_convertible<U, T>())
{
    qe::detail::assertConvertible<U, T>();
    if (*this != other)
        reset(other.take());
    return *this;
}

The is_convertible in the noexcept doesn’t make sense. It is non-throwing if what you do in the body will never throw. If this is a trick to get SFINAE tucked away, it will not work. Likewise, the assertConvertible does not belong in the body at all! You want this form of operator= to only match overloading if the conversion is possible.

That is, use std::enable_if, as an extra dummy template parameter.

Meanwhile, it doesn’t make sense to assign an int* to a long* without any kind of conversion going on. You want to check conversion ability on the pointers, not on the types being pointed to!

The style in C++ is to put the * or & with the type, not the identifier. This is called out specifically near the beginning of Stroustrup’s first book, and is an intentional difference from C style.

---

    auto oldD = this->d;
    this->d = nullptr;

Don’t use this-> for normal member access.

In this function, you could just write return std::exchange (d, nullptr);. See CPPreference — you mentioned needing to get to know the libraries better; this is a good resource to keep at hand.

Answer 3

comparison operators

You don’t need so many forms of the comparison operators.

First of all, a single template for != will reverse the sense of any == that you have, so don’t write all of them twice.

Second, don’t write special forms for nullptr. You are discouraged from writing explicit tests against nullptr and instead use the contextual conversion to bool and operator!. In fact, before C++11 you could not have special forms for this.

If you have another value that is null, the general form will be used; so, it has to handle the null case.

You can however provide a non-explicit constructor that converts nullptr to the smart pointer type.