Inplace construct error message in StatusOr while data type is large …

…enough (apache#1198)
MaximSmolskiy · Dec 21, 2022 · 4516b75 · 4516b75
1 parent cb49bd5
commit 4516b75
Show file tree

Hide file tree

Showing 2 changed files with 97 additions and 17 deletions.
diff --git a/src/common/status.h b/src/common/status.h
@@ -105,11 +105,61 @@ class [[nodiscard]] Status {
   friend struct StatusOr;
 };
 
+namespace type_details {
 template <typename... Ts>
-using first_element = typename std::tuple_element<0, std::tuple<Ts...>>::type;
+using first_element_t = typename std::tuple_element_t<0, std::tuple<Ts...>>;
 
 template <typename T>
-using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>::type>::type;
+using remove_cvref_t = typename std::remove_cv_t<typename std::remove_reference_t<T>>;
+};  // namespace type_details
+
+template <typename, typename = void>
+struct StringInStatusOr : private std::string {
+  using base_type = std::string;
+  static constexpr bool inplace = true;
+
+  explicit StringInStatusOr(std::string&& v) : base_type(std::move(v)) {}
+
+  template <typename U>
+  StringInStatusOr(StringInStatusOr<U>&& v) : base_type(*std::move(v)) {}  // NOLINT
+  StringInStatusOr(const StringInStatusOr& v) = delete;
+
+  StringInStatusOr& operator=(const StringInStatusOr&) = delete;
+
+  std::string& operator*() & { return *this; }
+
+  const std::string& operator*() const& { return *this; }
+
+  std::string&& operator*() && { return std::move(*this); }
+
+  ~StringInStatusOr() = default;
+};
+
+template <typename T>
+struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> : private std::unique_ptr<std::string> {
+  using base_type = std::unique_ptr<std::string>;
+  static constexpr bool inplace = false;
+
+  explicit StringInStatusOr(std::string&& v) : base_type(new std::string(std::move(v))) {}
+
+  template <typename U, typename std::enable_if_t<StringInStatusOr<U>::inplace, int> = 0>
+  StringInStatusOr(StringInStatusOr<U>&& v) : base_type(new std::string(*std::move(v))) {}  // NOLINT
+  template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
+  StringInStatusOr(StringInStatusOr<U>&& v)  // NOLINT
+      : base_type((typename StringInStatusOr<U>::base_type &&)(std::move(v))) {}
+
+  StringInStatusOr(const StringInStatusOr& v) = delete;
+
+  StringInStatusOr& operator=(const StringInStatusOr&) = delete;
+
+  std::string& operator*() & { return base_type::operator*(); }
+
+  const std::string& operator*() const& { return base_type::operator*(); }
+
+  std::string&& operator*() && { return std::move(base_type::operator*()); }
+
+  ~StringInStatusOr() = default;
+};
 
 template <typename>
 struct StatusOr;
@@ -121,36 +171,37 @@ template <typename T>
 struct IsStatusOr<StatusOr<T>> : std::integral_constant<bool, true> {};
 
 template <typename T>
-struct [[nodiscard]] StatusOr {  // NOLINT
-  static_assert(!std::is_same<T, Status>::value, "value_type cannot be Status");
-  static_assert(!std::is_same<T, Status::Code>::value, "value_type cannot be Status::Code");
+struct [[nodiscard]] StatusOr {
+  static_assert(!std::is_same_v<T, Status>, "value_type cannot be Status");
+  static_assert(!std::is_same_v<T, Status::Code>, "value_type cannot be Status::Code");
   static_assert(!IsStatusOr<T>::value, "value_type cannot be StatusOr");
-  static_assert(!std::is_reference<T>::value, "value_type cannot be reference");
+  static_assert(!std::is_reference_v<T>, "value_type cannot be reference");
 
   using value_type = T;
 
   // we use std::unique_ptr to make the error part as small as enough
-  using error_type = std::unique_ptr<std::string>;
+  using error_type = StringInStatusOr<value_type>;
 
   using Code = Status::Code;
 
   StatusOr(Status s) : code_(s.code_) {  // NOLINT
     CHECK(!s);
-    new (&error_) error_type(new std::string(std::move(s.msg_)));
+    new (&error_) error_type(std::move(s.msg_));
   }
 
   StatusOr(Code code, std::string msg = {}) : code_(code) {  // NOLINT
     CHECK(code != Code::cOK);
-    new (&error_) error_type(new std::string(std::move(msg)));
+    new (&error_) error_type(std::move(msg));
   }
 
   template <typename... Ts,
-            typename std::enable_if<(sizeof...(Ts) > 0 &&
-                                     !std::is_same<Status, remove_cvref_t<first_element<Ts...>>>::value &&
-                                     !std::is_same<Code, remove_cvref_t<first_element<Ts...>>>::value &&
-                                     !std::is_same<StatusOr, remove_cvref_t<first_element<Ts...>>>::value),
-                                    int>::type = 0>  // NOLINT
-  StatusOr(Ts&&... args) : code_(Code::cOK) {        // NOLINT
+            typename std::enable_if<
+                (sizeof...(Ts) > 0 &&
+                 !std::is_same_v<Status, type_details::remove_cvref_t<type_details::first_element_t<Ts...>>> &&
+                 !std::is_same_v<Code, type_details::remove_cvref_t<type_details::first_element_t<Ts...>>> &&
+                 !std::is_same_v<StatusOr, type_details::remove_cvref_t<type_details::first_element_t<Ts...>>>),
+                int>::type = 0>                // NOLINT
+  StatusOr(Ts&&... args) : code_(Code::cOK) {  // NOLINT
     new (&value_) value_type(std::forward<Ts>(args)...);
   }
 
@@ -183,12 +234,12 @@ struct [[nodiscard]] StatusOr {  // NOLINT
 
   Status ToStatus() const& {
     if (*this) return Status::OK();
-    return Status(code_, *error_);
+    return {code_, *error_};
   }
 
   Status ToStatus() && {
     if (*this) return Status::OK();
-    return Status(code_, std::move(*error_));
+    return {code_, std::move(*error_)};
   }
 
   operator Status() const& { return ToStatus(); }  // NOLINT

diff --git a/tests/cppunit/status_test.cc b/tests/cppunit/status_test.cc
@@ -163,3 +163,32 @@ TEST(StatusOr, ValueOr) {
   std::string s = "hi";
   ASSERT_EQ(StatusOr<std::string>(Status::NotOK, "").ValueOr(s), "hi");
 }
+
+TEST(StatusOr, Size) {
+  struct A {
+    std::string a, b;
+  };
+
+  static_assert(!StatusOr<char>::error_type::inplace);
+  static_assert(!StatusOr<int>::error_type::inplace);
+  static_assert(StatusOr<std::string>::error_type::inplace);
+  static_assert(StatusOr<A>::error_type::inplace);
+
+  struct B1 {
+    char a;
+    void *b;
+  };
+  struct B2 {
+    char a;
+    std::string b;
+  };
+  struct B3 {
+    char a;
+    A b;
+  };
+  static_assert(sizeof(StatusOr<char>) == sizeof(B1));
+  static_assert(sizeof(StatusOr<int>) == sizeof(B1));
+  static_assert(sizeof(StatusOr<void *>) == sizeof(B1));
+  static_assert(sizeof(StatusOr<std::string>) == sizeof(B2));
+  static_assert(sizeof(StatusOr<A>) == sizeof(B3));
+}