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
andstd::swap
, - declaring the expected type traits,
- and implementing all of the public member functions in
std::unique_pointer
.
- adding a partial specialization of
- To also maintain compatibility with Qt, so that
qe::UniquePointer
acts as a drop-in replacement forQScopedPointer
. - 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 ofstd::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.