[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #768: Add `StatusOr` for error handling in modern C++ style

[GitBox <gi...@apache.org>]

git-hulk commented on code in PR #768:
URL: https://github.com/apache/incubator-kvrocks/pull/768#discussion_r937358592


##########
src/status.h:
##########
@@ -61,20 +66,204 @@ class Status {
   };
 
   Status() : Status(cOK) {}
-  explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
-  bool IsOK() { return code_ == cOK; }
-  bool IsNotFound() { return code_ == NotFound; }
-  bool IsImorting() { return code_ == SlotImport; }
-  bool IsBlockingCommand() { return code_ == BlockingCmd; }
-  std::string Msg() {
-    if (IsOK()) {
-      return "ok";
-    }
+  explicit Status(Code code, std::string msg = {})
+    : code_(code), msg_(std::move(msg)) {}
+
+  template <Code code>
+  bool Is() const { return code_ == code; }
+
+  bool IsOK() const { return Is<cOK>(); }
+  operator bool() const { return IsOK(); }
+
+  Code GetCode() const {
+    return code_;
+  }
+
+  std::string Msg() const& {
+    if (*this) return ok_msg();
     return msg_;
   }
+
+  std::string Msg() && {
+    if (*this) return ok_msg();
+    return std::move(msg_);
+  }
+
   static Status OK() { return {}; }
 
  private:
   Code code_;
   std::string msg_;
+
+  static constexpr const char* ok_msg() {
+    return "ok";
+  }
+
+  template <typename T>
+  friend struct StatusOr;
+};
+
+template <typename ...Ts>
+using first_element = typename std::tuple_element<0, std::tuple<Ts...>>::type;
+
+template <typename T>
+using remove_cvref_t = typename std::remove_cv<typename std::remove_reference<T>::type>::type;
+
+template <typename T>
+struct StatusOr;
+
+template <typename T>
+struct IsStatusOr : std::integral_constant<bool, false> {};
+
+template <typename T>
+struct IsStatusOr<StatusOr<T>> : std::integral_constant<bool, true> {};
+
+template <typename T>
+struct StatusOr {
+  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");
+  static_assert(!IsStatusOr<T>::value, "value_type cannot be StatusOr");
+  static_assert(!std::is_reference<T>::value, "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 Code = Status::Code;
+
+  explicit StatusOr(Status s) : code_(s.code_) {
+    CHECK(!s);
+    new(storage_) error_type(new std::string(std::move(s.msg_)));
+  }
+
+  StatusOr(Code code, std::string msg = {}) : code_(code) { // NOLINT
+    CHECK(code != Code::cOK);
+    new(storage_) error_type(new std::string(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<value_type, remove_cvref_t<first_element<Ts...>>>::value &&
+        !std::is_same<StatusOr, remove_cvref_t<first_element<Ts...>>>::value
+      ), int>::type = 0> // NOLINT
+  explicit StatusOr(Ts && ... args) : code_(Code::cOK) {
+    new(storage_) value_type(std::forward<Ts>(args)...);
+  }
+
+  StatusOr(T&& value) : code_(Code::cOK) { // NOLINT
+    new(storage_) value_type(std::move(value));
+  }
+
+  StatusOr(const T& value) : code_(Code::cOK) { // NOLINT
+    new(storage_) value_type(value);
+  }
+
+  StatusOr(const StatusOr&) = delete;
+  StatusOr(StatusOr&& other) : code_(other.code_) {
+    if (code_ == Code::cOK) {
+      new(storage_) value_type(std::move(other.getValue()));
+    } else {
+      new(storage_) error_type(std::move(other.getError()));
+    }
+  }
+
+  Status& operator=(const Status&) = delete;
+
+  template <Code code>
+  bool Is() const { return code_ == code; }
+
+  bool IsOK() const { return Is<Code::cOK>(); }
+  operator bool() const { return IsOK(); }
+
+  Status ToStatus() const& {
+    if (*this) return Status::OK();
+    return Status(code_, *getError());
+  }
+
+  Status ToStatus() && {
+    if (*this) return Status::OK();
+    return Status(code_, std::move(*getError()));
+  }
+
+  Code GetCode() const {
+    return code_;
+  }
+
+  value_type& GetValue() & {
+    CHECK(*this);
+    return getValue();
+  }
+
+  value_type&& GetValue() && {
+    CHECK(*this);
+    return std::move(getValue());
+  }
+
+  const value_type& GetValue() const& {
+    CHECK(*this);
+    return getValue();
+  }
+
+  value_type& operator*() & {
+    return GetValue();
+  }
+
+  value_type&& operator*() && {
+    return std::move(GetValue());
+  }
+
+  const value_type& operator*() const& {
+    return GetValue();
+  }
+
+  value_type* operator->() {
+    return &GetValue();
+  }
+
+  const value_type* operator->() const {
+    return &GetValue();
+  }
+
+  std::string Msg() const& {
+    if (*this) return Status::ok_msg();
+    return *getError();
+  }
+
+  std::string Msg() && {
+    if (*this) return Status::ok_msg();
+    return std::move(*getError());
+  }
+
+  ~StatusOr() {
+    if (*this) {
+      getValue().~value_type();
+    } else {
+      getError().~error_type();
+    }
+  }
+
+ private:
+  Status::Code code_;
+  alignas(value_type) alignas(error_type) unsigned char storage_

Review Comment:
   Is it a good idea to use `value_` instead of `storage_` here? coz we used the `storage_` heavily in `storage.cc` but it's totally different. So I'm wondering if this will
   confuse others.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org