-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BALDE-87] Implement error handling system. #70
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # engine/src/core/utils/file_manager.cpp
fixed error
engine/src/core/error/unexpected.h
Outdated
struct unexpected { | ||
constexpr explicit unexpected(const T& val): value(val) { } | ||
constexpr operator T() const noexcept { return value; } | ||
T value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a rationale as to why this is public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean "value" member.
engine/src/core/error/expected.h
Outdated
constexpr expected(const E& rhs) { new(&valid) E(rhs); } | ||
|
||
constexpr expected(const unexpected<U>& rhs) : | ||
_isOk(false) { new(&invalid) U(rhs.value); } | ||
|
||
constexpr expected(E&& rhs) { new(&valid) E(std::forward<E>(rhs)); } | ||
|
||
constexpr expected(unexpected<U>&& rhs) : | ||
_isOk(false) { new(&invalid) U(std::forward<U>(rhs.value)); } | ||
|
||
constexpr expected(const expected<E, U>& rhs) : | ||
_isOk(rhs._isOk) { | ||
if (rhs._isOk) new(&valid) E(rhs.valid); | ||
else new(&invalid) U(rhs.invalid); | ||
} | ||
|
||
constexpr expected(expected<E, U>&& rhs) noexcept : | ||
_isOk(rhs._isOk) { | ||
if (rhs._isOk) new(&valid) E(std::move(rhs.valid)); | ||
else new(&invalid) U(std::move(rhs.invalid)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make all single argument constructors explicit. Also, check for noexcept correctness. I believe placement new can throw an exception, so take one more look at line 39.
SMALL_FILE, BIG_FILE | ||
}; | ||
|
||
enum class Error : uint8_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss about the error type. I think having a global type would be interesting as well (we could write a constructor overload for this error type and be able to return from function Error::SomeError instead of unexpected(FileManager::Error::CantOpenFile))
engine/src/core/error/expected.h
Outdated
else new(&invalid) U(std::move(rhs.invalid)); | ||
} | ||
|
||
constexpr expected<E, U>& operator=(const expected<E, U>& other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be consistent. If you use "rhs" to refer to rand hand side of the assignment, keep it consistent across the implementation. Same applies to methods defined below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You provide an implementation for assignment operator with "expected" as an argument for both L-val and R-val reference, which is good since you also provide appropriate constructors. However, there is no assignment operator provided for L-val/R-val for the "unexpected", but the constructors for these types are provided. Please elaborate on this.
engine/src/core/error/expected.h
Outdated
[[nodiscard]] constexpr bool has_value() const noexcept { return _isOk; } | ||
|
||
[[nodiscard]] constexpr bool isOk() const noexcept { return _isOk; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are essentially the same. Make one, and possibly name it isValid() to match the inner member.
…g_system' into BALDE_87_Implement_error_handling_system
…ng isValid function or conversion to bool
# Conflicts: # engine/src/core/events/callback/event_function_handler.h
No description provided.