refactor: use new return type internally#72
Conversation
| SFrameError(const SFrameError& other) | ||
| : type_(SFrameErrorType::none) | ||
| , message_(other.message_) | ||
| { | ||
| type_ = other.type_; | ||
| } |
There was a problem hiding this comment.
Why does this need to be non-default? Similar question with several of the methods below.
|
|
||
| private: | ||
| SFrameErrorType type_ = SFrameErrorType::none; | ||
| std::string message_; |
There was a problem hiding this comment.
I don't love having STL containers here. Since we have a fixed list of messages, let's just define const char* constants for them and refer to them. Especially since that's how we're exposing them anyway!
| // Error types to replace exceptions | ||
| enum class SFrameErrorType | ||
| { | ||
| none = 0, |
There was a problem hiding this comment.
I would remove this option (but still start numbering at 1) and the SFrameError::ok() method.
| } | ||
|
|
||
| // Copy assignment | ||
| SFrameError& operator=(const SFrameError& other) |
There was a problem hiding this comment.
Not clear that this type needs to be assignable. If it causes conflict with the const char* note below, I would drop the operator=.
| void | ||
| throw_on_error(const SFrameError& error); |
There was a problem hiding this comment.
Instead of doing this, I would have a method Result::unwrap that throws on error. This also works well with eliminating SFrameErrorType::none. And it will save you having to write branches on is_ok().
| Result(Result&& other) noexcept | ||
| : data_(std::move(other.data_)) | ||
| { | ||
| } | ||
|
|
||
| Result& operator=(Result&& other) noexcept | ||
| { | ||
| data_ = std::move(other.data_); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Are these not the same as the default?
| private: | ||
| std::variant<T, SFrameError> data_; |
There was a problem hiding this comment.
Minor note: If T = SFrameError, I think this class will fail to compile and/or produce weird results (e.g., from is_ok / is_err). I don't think this is a case we need to accommodate, though.
| return *this; | ||
| } | ||
|
|
||
| T value() { return std::move(std::get<T>(data_)); } |
There was a problem hiding this comment.
I would follow Rust here and call this unwrap. Note that it has "throw if error" semantics anyway, because if it contains an error, std::get will throw bad_variant_access if is_err.
| bool is_ok_; | ||
| SFrameError error_; |
There was a problem hiding this comment.
I would replace these with std::optional<SFrameError>. Even less need for SFrameError::none.
| auto decode_result = Header::parse(tc.encoding); | ||
| REQUIRE(decode_result.is_ok()); | ||
| const auto decoded = decode_result.value(); |
There was a problem hiding this comment.
Once you have unwrap(), these three lines will compress back to:
const auto decoded = Header::parse(tc.encoding).unwrap();
... since unwrap will throw and fail the test if is_err.
2 participants
No description provided.