From 84e9889454229ff2410a7f730f40dcb12a96be81 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi@scylladb.com>
Date: Mon, 23 Dec 2024 19:51:45 +0200
Subject: [PATCH] future: avoid inheriting from future payload type

The uninitialized_wrapper_base will, when possible inherit from
the future payload type T in order to apply the empty base
optimization (if sizeof(T) == 0). However, this injects methods
from T into the scope of uninitialized_wrapper and classes
that inherit from it, such as future_state. If one of the names
of the methods in future_state clashes with a member of T, we
get a compilation failure.

Fix by avoiding the inheritance branch of uninitialized_wrapper_base
and using the composition branch, and use [[no_unique_address]]
to avoid bloating future<some_empty_class>.

It turns out that we delete the specialization that was used
with internal::monostate (corresponding to future<>), and so
we have to specialize uninitialized_wrapper for it.

I verified that sizeof(future<>) isn't changed by the patch.

Closes scylladb/seastar#2598
---
 include/seastar/core/future.hh | 57 +++++++++++-----------------------
 1 file changed, 18 insertions(+), 39 deletions(-)

diff --git a/include/seastar/core/future.hh b/include/seastar/core/future.hh
index cc5c91220a..d24e7d48e9 100644
--- a/include/seastar/core/future.hh
+++ b/include/seastar/core/future.hh
@@ -286,24 +286,19 @@ using maybe_wrap_ref = std::conditional_t<std::is_reference_v<T>, std::reference
 ///
 /// This is similar to a std::optional<T>, but it doesn't know if it is holding a value or not, so the user is
 /// responsible for calling constructors and destructors.
-///
-/// The advantage over just using a union directly is that this uses inheritance when possible and so benefits from the
-/// empty base optimization.
-template <typename T, bool is_trivial_class>
-struct uninitialized_wrapper_base;
 
 template <typename T>
-struct uninitialized_wrapper_base<T, false> {
+struct uninitialized_wrapper {
     using tuple_type = future_tuple_type_t<T>;
-    union any {
+    [[no_unique_address]] union any {
         any() noexcept {}
         ~any() {}
         // T can be a reference, so wrap it.
-        maybe_wrap_ref<T> value;
+        [[no_unique_address]] maybe_wrap_ref<T> value;
     } _v;
 
 public:
-    uninitialized_wrapper_base() noexcept = default;
+    uninitialized_wrapper() noexcept = default;
     template<typename... U>
     std::enable_if_t<!std::is_same_v<std::tuple<std::remove_cv_t<U>...>, std::tuple<tuple_type>>, void>
     uninitialized_set(U&&... vs) {
@@ -323,43 +318,27 @@ public:
     }
 };
 
-template <typename T> struct uninitialized_wrapper_base<T, true> : private T {
-    using tuple_type = future_tuple_type_t<T>;
-    uninitialized_wrapper_base() noexcept = default;
-    template<typename... U>
-    std::enable_if_t<!std::is_same_v<std::tuple<std::remove_cv_t<U>...>, std::tuple<tuple_type>>, void>
-    uninitialized_set(U&&... vs) {
-        new (this) T(std::forward<U>(vs)...);
+template <>
+struct uninitialized_wrapper<internal::monostate> {
+    [[no_unique_address]] internal::monostate _v;
+public:
+    uninitialized_wrapper() noexcept = default;
+    void uninitialized_set() {
     }
-    void uninitialized_set(tuple_type&& v) {
-        if constexpr (std::tuple_size_v<tuple_type> != 0) {
-            uninitialized_set(std::move(std::get<0>(v)));
-        }
+    void uninitialized_set(internal::monostate) {
     }
-    void uninitialized_set(const tuple_type& v) {
-        if constexpr (std::tuple_size_v<tuple_type> != 0) {
-            uninitialized_set(std::get<0>(v));
-        }
+    void uninitialized_set(std::tuple<>&& v) {
     }
-    T& uninitialized_get() {
-        return *this;
+    void uninitialized_set(const std::tuple<>& v) {
     }
-    const T& uninitialized_get() const {
-        return *this;
+    internal::monostate& uninitialized_get() {
+        return _v;
+    }
+    const internal::monostate& uninitialized_get() const {
+        return _v;
     }
 };
 
-template <typename T>
-constexpr bool can_inherit =
-        (std::is_trivially_destructible_v<T> && std::is_trivially_constructible_v<T> &&
-                std::is_class_v<T> && !std::is_final_v<T>);
-
-// The objective is to avoid extra space for empty types like std::tuple<>. We could use std::is_empty_v, but it is
-// better to check that both the constructor and destructor can be skipped.
-template <typename T>
-struct uninitialized_wrapper
-    : public uninitialized_wrapper_base<T, can_inherit<T>> {};
-
 template <typename T>
 struct is_trivially_move_constructible_and_destructible {
     static constexpr bool value = std::is_trivially_move_constructible_v<T> && std::is_trivially_destructible_v<T>;