Produce better warnings for unique pointers

Currently, we rely unnecessarily heavily on SFINAE for our unique
pointer implementation, which leads to extremely confusing error
messages. This commit migrates these errors and checks to use static
asserts, which allow us to produce more helpful messages for our end
users.

core/CMakeLists.txt

@@ -83,8 +83,6 @@ vecmem_add_library( vecmem_core core
  "include/vecmem/memory/conditional_memory_resource.hpp"
  "src/memory/debug_memory_resource.cpp"
  "include/vecmem/memory/debug_memory_resource.hpp"
- "src/memory/details/unique_alloc_deleter_impl.cpp"
- "include/vecmem/memory/details/unique_alloc_deleter_impl.hpp"
  "include/vecmem/memory/details/unique_alloc_deleter.hpp"
  "include/vecmem/memory/details/unique_obj_deleter.hpp"
  "include/vecmem/memory/unique_ptr.hpp"

core/include/vecmem/memory/details/unique_alloc_deleter.hpp

@@ -10,12 +10,125 @@
 
 #include <type_traits>
 
-#include "vecmem/memory/details/unique_alloc_deleter_impl.hpp"
+#include "vecmem/memory/memory_resource.hpp"
+#include "vecmem/vecmem_core_export.hpp"
 
 namespace vecmem::details {
+/**
+ * @brief A deleter class for trivial allocations.
+ *
+ * This class provides all the necessary functionality to allow unique pointers
+ * to deallocate allocations of trivial types, as well as arrays of them.
+ *
+ * @tparam T The type to deallocate.
+ *
+ * @note This deleter class supports non-array types, array types of unknown
+ * bound, but not array types of known bound.
+ *
+ * @note This deleter class only supports types that are both trivially
+ * constructible and trivially destructible.
+ */
 template <typename T>
-using unique_alloc_deleter = std::enable_if_t<
- std::is_trivially_constructible_v<std::remove_extent_t<T>> &&
- std::is_trivially_destructible_v<std::remove_extent_t<T>>,
- unique_alloc_deleter_impl>;
-}
+struct unique_alloc_deleter {
+ /*
+ * When an allocation happens on a device, we are not guaranteed to be able
+ * to write to it, so we don't construct any objects in that memory. Thus,
+ * we need to ensure that the memory is in a valid state by requiring that
+ * the types can start their lifetime without construction.
+ */
+ static_assert(std::is_trivially_constructible_v<std::remove_extent_t<T>>,
+ "Allocation pointer type must be trivially constructible.");
+
+ /*
+ * Similarly, we cannot destroy objects, so we need to ensure that
+ * deallocation of objects is semantically equivalent to their destruction.
+ */
+ static_assert(std::is_trivially_destructible_v<std::remove_extent_t<T>>,
+ "Allocation pointer type must be trivially destructible.");
+
+public:
+ /**
+ * @brief Default-construct a new unique allocation deleter.
+ *
+ * This constructor should only really be used in cases where we are
+ * nulling a unique pointer.
+ */
+ unique_alloc_deleter(void) = default;
+
+ /**
+ * @brief Construct a new unique allocation deleter.
+ *
+ * This is the standard constructor for this class, which stores all the
+ * necessary data to deallocate an allocation.
+ *
+ * @param mr The memory resource to use for deallocation.
+ * @param p The pointer to deallocate.
+ * @param s The size of the allocation.
+ * @param a The alignment of the allocation.
+ */
+ unique_alloc_deleter(memory_resource& mr, std::size_t s, std::size_t a = 0)
+ : m_mr(&mr), m_size(s), m_align(a) {}
+
+ /**
+ * @brief Copy a unique allocation deleter.
+ *
+ * @param i The allocation deleter to copy.
+ */
+ unique_alloc_deleter(const unique_alloc_deleter& i) = default;
+
+ /**
+ * @brief Move a unique allocation deleter.
+ *
+ * @param i The allocation deleter to move.
+ */
+ unique_alloc_deleter(unique_alloc_deleter&& i) = default;
+
+ /**
+ * @brief Copy-assign a unique allocation deleter.
+ *
+ * @param i The object to copy into the current one.
+ *
+ * @return A reference to this object.
+ */
+ unique_alloc_deleter& operator=(const unique_alloc_deleter& i) = default;
+
+ /**
+ * @brief Move-assign a unique allocation deleter.
+ *
+ * @param i The object to move into the current one.
+ *
+ * @return A reference to this object.
+ */
+ unique_alloc_deleter& operator=(unique_alloc_deleter&& i) = default;
+
+ /**
+ * @brief Activate the deletion mechanism of the deleter.
+ *
+ * @param p The pointer to deallocate.
+ */
+ void operator()(void* p) const {
+ assert(m_mr != nullptr);
+
+ /*
+ * As before, if this happens... Something has gone VERY wrong.
+ */
+ if (m_mr == nullptr) {
+ return;
+ }
+
+ /*
+ * Deallocate the memory that we were using.
+ */
+ if (m_align > 0) {
+ m_mr->deallocate(p, m_size, m_align);
+ } else {
+ m_mr->deallocate(p, m_size);
+ }
+ }
+
+private:
+ memory_resource* m_mr;
+ std::size_t m_size;
+ std::size_t m_align;
+};
+} // namespace vecmem::details

core/include/vecmem/memory/details/unique_obj_deleter.hpp

@@ -35,12 +35,14 @@ namespace vecmem::details {
  * cannot check this at compile-time or at run-time, so it is the
  * responsibility of the user to ensure that this requirement is respected.
  */
-template <typename T,
- std::enable_if_t<!std::is_array_v<T> ||
- (std::is_array_v<T> && std::extent_v<T> == 0),
- bool> = true>
+template <typename T>
 struct unique_obj_deleter {
 public:
+ static_assert(!std::is_array_v<T> ||
+ (std::is_array_v<T> && std::extent_v<T> == 0),
+ "Pointer type of unique object must be either non-array or "
+ "unbound array type.");
+
  using pointer_t =
  std::conditional_t<std::is_array_v<T>, std::decay_t<T>, T*>;
  using storage_t = std::remove_pointer_t<pointer_t>;

core/include/vecmem/memory/unique_ptr.hpp

@@ -153,9 +153,13 @@ make_unique_obj(memory_resource& m, std::size_t n) {
  * constructible and destructible.
  */
 template <typename T>
-typename std::enable_if_t<!(std::is_array_v<T> && std::extent_v<T> == 0),
- unique_alloc_ptr<T>>
-make_unique_alloc(memory_resource& m) {
+unique_alloc_ptr<T> make_unique_alloc(memory_resource& m) {
+ /*
+ * This method only works on non-array types and bounded array types.
+ */
+ static_assert(!(std::is_array_v<T> && std::extent_v<T> == 0),
+ "Allocation pointer type cannot be an unbounded array.");
+
  using pointer_t =
  std::conditional_t<std::is_array_v<T>, std::decay_t<T>, T*>;
 
@@ -187,9 +191,15 @@ make_unique_alloc(memory_resource& m) {
  * @return A unique allocation pointer to a newly allocated array.
  */
 template <typename T>
-typename std::enable_if_t<std::is_array_v<T> && std::extent_v<T> == 0,
- unique_alloc_ptr<T>>
-make_unique_alloc(memory_resource& m, std::size_t n) {
+unique_alloc_ptr<T> make_unique_alloc(memory_resource& m, std::size_t n) {
+ /*
+ * This overload only works for unbounded array types.
+ */
+ static_assert(std::is_array_v<T>,
+ "Allocation pointer type must be an array type.");
+ static_assert(std::extent_v<T> == 0,
+ "Allocation pointer type must be unbounded.");
+
  using pointer_t =
  std::conditional_t<std::is_array_v<T>, std::decay_t<T>, T*>;