Merge pull request #405 from StreamHPC/fix-optional-emplace

Fix optional emplace

CHANGELOG.md

@@ -17,6 +17,9 @@ Documentation for rocThrust available at
 * Updated internal use of custom iterator in `thrust::detail::unique_by_key` to use rocPRIM's `rocprim::unique_by_key`.
 * Updated `adjecent_difference` to make use of `rocprim:adjecent_difference` when iterators are comparable and not equal otherwise use `rocprim:adjacent_difference_inplace`.
 
+### Fixes
+* Fixed incorrect implementation of `thrust::optional<T&>::emplace()`.
+
 ### Known issues
 * `thrust::reduce_by_key` outputs are not bit-wise reproducible, as run-to-run results for pseudo-associative reduction operators (e.g. floating-point arithmetic operators) are not deterministic on the same device.
 
@@ -47,7 +50,7 @@ Documentation for rocThrust available at
 
 ## rocThrust 2.18.0 for ROCm 5.7
 
-### Fixes 
+### Fixes
 * `lower_bound`, `upper_bound`, and `binary_search` failed to compile for certain types.
 * Fixed issue where `transform_iterator` would not compile with `__device__`-only operators.
 

test/test_optional.cpp

@@ -28,3 +28,41 @@ TEST(OptionalTests, IsTriviallyCopyable)
     static_assert(std::is_trivially_copyable<thrust::optional<uint64_t>>::value == true,
                   "type is not trivially copyable even though it should be!");
 }
+
+TEST(OptionalTests, EmplaceReference)
+{
+    // See https://github.com/ROCm/rocThrust/issues/404
+    {
+        int a = 10;
+
+        thrust::optional<int&> maybe(a);
+
+        int b = 20;
+        maybe.emplace(b);
+
+        ASSERT_EQ(maybe.value(), 20);
+        // Emplacing with b shouldn't change a
+        ASSERT_EQ(a, 10);
+
+        int c = 30;
+        maybe.emplace(c);
+
+        ASSERT_EQ(maybe.value(), 30);
+        ASSERT_EQ(b, 20);
+    }
+
+    {
+        thrust::optional<int&> maybe;
+
+        int b = 21;
+        maybe.emplace(b);
+
+        ASSERT_EQ(maybe.value(), 21);
+
+        int c = 31;
+        maybe.emplace(c);
+
+        ASSERT_EQ(maybe.value(), 31);
+        ASSERT_EQ(b, 21);
+    }
+}

thrust/optional.h

@@ -2746,14 +2746,11 @@ template <class T> class optional<T &> {
   ///
   /// \group emplace
   __thrust_exec_check_disable__
-  template <class... Args>
+  template <class U>
   __host__ __device__
-  T &emplace(Args &&... args) noexcept {
-    static_assert(std::is_constructible<T, Args &&...>::value,
-                  "T must be constructible with Args");
-
-    *this = nullopt;
-    this->construct(std::forward<Args>(args)...);
+  T &emplace(U& u) noexcept {
+    m_value = addressof(u);
+    return *m_value;
   }
 
   /// Swaps this optional with the other.