[RFC] Add tests and rudimentary protections for default-constructed PortableCollections

DataFormats/Common/interface/DeviceProduct.h

@@ -3,6 +3,7 @@
 
 #include <cassert>
 #include <memory>
+#include <optional>
 
 namespace edm {
   class DeviceProductBase {
@@ -49,7 +50,7 @@ namespace edm {
 
     template <typename M, typename... Args>
     explicit DeviceProduct(std::shared_ptr<M> metadata, Args&&... args)
-        : DeviceProductBase(std::move(metadata)), data_(std::forward<Args>(args)...) {}
+        : DeviceProductBase(std::move(metadata)), data_(std::in_place_t{}, std::forward<Args>(args)...) {}
 
     DeviceProduct(const DeviceProduct&) = delete;
     DeviceProduct& operator=(const DeviceProduct&) = delete;
@@ -66,11 +67,14 @@ namespace edm {
     T const& getSynchronized(Args&&... args) const {
       auto const& md = metadata<M>();
       md.synchronize(std::forward<Args>(args)...);
-      return data_;
+      return *data_;
     }
 
   private:
-    T data_;  //!
+    // Using optional to allow T to not have a default constructor
+    // Default-constructed DeviceProduct should in any case appear
+    // only when edm::Wrapper::present == false
+    std::optional<T> data_;  //!
   };
 }  // namespace edm
 #endif

DataFormats/Common/test/BuildFile.xml

@@ -58,3 +58,7 @@
 <bin file="StdArray_t.cpp">
   <use name="catch2"/>
 </bin>
+
+<bin file="DeviceProduct_t.cpp">
+  <use name="catch2"/>
+</bin>

DataFormats/Common/test/DeviceProduct_t.cpp

@@ -0,0 +1,70 @@
+#define CATCH_CONFIG_MAIN
+#include "catch.hpp"
+
+#include "DataFormats/Common/interface/DeviceProduct.h"
+
+namespace {
+  class Metadata {
+  public:
+    Metadata(int v = 0) : value_(v) {}
+    void synchronize(int& ret) const { ret = value_; }
+
+  private:
+    int const value_;
+  };
+
+  class Product {
+  public:
+    Product() = default;
+    Product(int v) : value_(v) {}
+
+    int value() const { return value_; }
+
+  private:
+    int value_ = 0;
+  };
+
+  class ProductNoDefault {
+  public:
+    ProductNoDefault(int v) : value_(v) {}
+
+    int value() const { return value_; }
+
+  private:
+    int value_;
+  };
+
+}  // namespace
+
+TEST_CASE("DeviceProduct", "[DeviceProduct]") {
+  SECTION("Default constructor") { [[maybe_unused]] edm::DeviceProduct<int> prod; }
+
+  SECTION("Default-constructible data product") {
+    SECTION("Default constructed") {
+      edm::DeviceProduct<Product> prod(std::make_shared<Metadata>(3));
+
+      int md = 0;
+      auto const& data = prod.getSynchronized<Metadata>(md);
+      REQUIRE(md == 3);
+      REQUIRE(data.value() == 0);
+    }
+
+    SECTION("Explicitly constructed") {
+      edm::DeviceProduct<Product> prod(std::make_shared<Metadata>(4), 42);
+
+      int md = 0;
+      auto const& data = prod.getSynchronized<Metadata>(md);
+      REQUIRE(md == 4);
+      REQUIRE(data.value() == 42);
+    }
+  }
+
+  SECTION("Non-default-constructible data product") {
+    edm::DeviceProduct<ProductNoDefault> prod(std::make_shared<Metadata>(5), 314);
+
+    int md = 0;
+    auto const& data = prod.getSynchronized<Metadata>(md);
+    REQUIRE(md == 5);
+    REQUIRE(data.value() == 314);
+  }
+}

DataFormats/Portable/interface/PortableCollection.h

@@ -71,6 +71,7 @@ namespace cms::alpakatools {
     template <typename TQueue>
     static auto copyAsync(TQueue& queue, PortableHostCollection<TLayout> const& srcData) {
       using TDevice = typename alpaka::trait::DevType<TQueue>::type;
+      assert(srcData.isValid());
       PortableDeviceCollection<TLayout, TDevice> dstData(srcData->metadata().size(), queue);
       alpaka::memcpy(queue, dstData.buffer(), srcData.buffer());
       return dstData;

DataFormats/Portable/interface/PortableDeviceCollection.h

@@ -24,23 +24,21 @@ class PortableDeviceCollection {
   using Buffer = cms::alpakatools::device_buffer<TDev, std::byte[]>;
   using ConstBuffer = cms::alpakatools::const_device_buffer<TDev, std::byte[]>;
 
-  PortableDeviceCollection() = default;
-
   PortableDeviceCollection(int32_t elements, TDev const& device)
       : buffer_{cms::alpakatools::make_device_buffer<std::byte[]>(device, Layout::computeDataSize(elements))},
-        layout_{buffer_->data(), elements},
+        layout_{buffer_.data(), elements},
         view_{layout_} {
     // Alpaka set to a default alignment of 128 bytes defining ALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128
-    assert(reinterpret_cast<uintptr_t>(buffer_->data()) % Layout::alignment == 0);
+    assert(reinterpret_cast<uintptr_t>(buffer_.data()) % Layout::alignment == 0);
   }
 
   template <typename TQueue, typename = std::enable_if_t<alpaka::isQueue<TQueue>>>
   PortableDeviceCollection(int32_t elements, TQueue const& queue)
       : buffer_{cms::alpakatools::make_device_buffer<std::byte[]>(queue, Layout::computeDataSize(elements))},
-        layout_{buffer_->data(), elements},
+        layout_{buffer_.data(), elements},
         view_{layout_} {
     // Alpaka set to a default alignment of 128 bytes defining ALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128
-    assert(reinterpret_cast<uintptr_t>(buffer_->data()) % Layout::alignment == 0);
+    assert(reinterpret_cast<uintptr_t>(buffer_.data()) % Layout::alignment == 0);
   }
 
   // non-copyable
@@ -54,6 +52,12 @@ class PortableDeviceCollection {
   // default destructor
   ~PortableDeviceCollection() = default;
 
+  // has a valid buffer?
+  bool isValid() const { return true; }
+
+  // a way to access the number of elements without the buffer validity assertion
+  auto size() const { return view_.metadata().size(); }
+
   // access the View
   View& view() { return view_; }
   ConstView const& view() const { return view_; }
@@ -66,14 +70,14 @@ class PortableDeviceCollection {
   ConstView const* operator->() const { return &view_; }
 
   // access the Buffer
-  Buffer buffer() { return *buffer_; }
-  ConstBuffer buffer() const { return *buffer_; }
-  ConstBuffer const_buffer() const { return *buffer_; }
+  Buffer buffer() { return buffer_; }
+  ConstBuffer buffer() const { return buffer_; }
+  ConstBuffer const_buffer() const { return buffer_; }
 
 private:
-  std::optional<Buffer> buffer_;  //!
-  Layout layout_;                 //
-  View view_;                     //!
+  Buffer buffer_;  //!
+  Layout layout_;  //
+  View view_;      //!
 };
 
 // generic SoA-based product in device memory

DataFormats/Portable/interface/PortableHostCollection.h

@@ -12,6 +12,9 @@
 #include "DataFormats/Portable/interface/PortableCollectionCommon.h"
 
 // generic SoA-based product in host memory
+//
+// Moving from PortableHostCollection leaves the source object in a
+// "not isValid()" state that has 0 size
 template <typename T>
 class PortableHostCollection {
 public:
@@ -47,27 +50,86 @@ class PortableHostCollection {
   PortableHostCollection& operator=(PortableHostCollection const&) = delete;
 
   // movable
-  PortableHostCollection(PortableHostCollection&&) = default;
-  PortableHostCollection& operator=(PortableHostCollection&&) = default;
+  PortableHostCollection(PortableHostCollection&& other):
+    buffer_(std::move(other.buffer_)),
+    layout_(std::move(other.layout_)),
+    view_(std::move(other.view_))
+          {
+            other.buffer_.reset();
+            other.view_ = View();
+          }
+  PortableHostCollection& operator=(PortableHostCollection&& other) {
+    // Protection for self-assignment without if-statements, following
+    // what was suggested in the C++ core guidelines
+    {
+      auto tmp = std::move(other.buffer_);
+      other.buffer_.reset();
+      buffer_.reset();
+      buffer_ = std::move(tmp);
+    }
+    {
+      auto tmp = std::move(other.view_);
+      other.view_ = View();
+      view_ = View();
+      view_ = std::move(tmp);
+    }
+    return *this;
+  }
 
   // default destructor
   ~PortableHostCollection() = default;
 
+  // has a valid buffer?
+  bool isValid() const { return buffer_.has_value(); }
+
+  // a way to access the number of elements without the buffer validity assertion
+  auto size() const { return view_.metadata().size(); }
+
   // access the View
-  View& view() { return view_; }
-  ConstView const& view() const { return view_; }
-  ConstView const& const_view() const { return view_; }
+  View& view() {
+    assert(isValid());
+    return view_;
+  }
+  ConstView const& view() const {
+    assert(isValid());
+    return view_;
+  }
+  ConstView const& const_view() const {
+    assert(isValid());
+    return view_;
+  }
 
-  View& operator*() { return view_; }
-  ConstView const& operator*() const { return view_; }
+  View& operator*() {
+    assert(isValid());
+    return view_;
+  }
+  ConstView const& operator*() const {
+    assert(isValid());
+    return view_;
+  }
 
-  View* operator->() { return &view_; }
-  ConstView const* operator->() const { return &view_; }
+  View* operator->() {
+    assert(isValid());
+    return &view_;
+  }
+  ConstView const* operator->() const {
+    assert(isValid());
+    return &view_;
+  }
 
   // access the Buffer
-  Buffer buffer() { return *buffer_; }
-  ConstBuffer buffer() const { return *buffer_; }
-  ConstBuffer const_buffer() const { return *buffer_; }
+  Buffer buffer() {
+    assert(isValid());
+    return *buffer_;
+  }
+  ConstBuffer buffer() const {
+    assert(isValid());
+    return *buffer_;
+  }
+  ConstBuffer const_buffer() const {
+    assert(isValid());
+    return *buffer_;
+  }
 
   // part of the ROOT read streamer
   static void ROOTReadStreamer(PortableHostCollection* newObj, Layout& layout) {

DataFormats/Portable/test/BuildFile.xml

@@ -3,3 +3,12 @@
   <use name="DataFormats/SoATemplate"/>
   <use name="catch2"/>
 </bin>
+
+<bin name="TestDataFormatsPortableCollection" file="alpaka/test_catch2_*.cc">
+  <use name="DataFormats/Portable"/>
+  <use name="DataFormats/SoATemplate"/>
+  <use name="HeterogeneousCore/AlpakaInterface"/>
+  <use name="alpaka"/>
+  <use name="catch2"/>
+  <flags ALPAKA_BACKENDS="1"/>
+</bin>

DataFormats/Portable/test/alpaka/test_catch2_main.cc

@@ -0,0 +1,2 @@
+#define CATCH_CONFIG_MAIN
+#include <catch.hpp>