Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

BIGbadEL
Copy link
Collaborator

No description provided.

@BIGbadEL BIGbadEL added the do not merge yet please, don't do it! label Sep 18, 2019
@BIGbadEL BIGbadEL self-assigned this Sep 18, 2019
@BIGbadEL BIGbadEL requested a review from Blinkuu September 19, 2019 15:08
@BIGbadEL BIGbadEL removed the do not merge yet please, don't do it! label Sep 26, 2019
engine/src/core/error/expected.h Show resolved Hide resolved
struct unexpected {
constexpr explicit unexpected(const T& val): value(val) { }
constexpr operator T() const noexcept { return value; }
T value;
Copy link
Collaborator

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?

Copy link
Collaborator

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 Show resolved Hide resolved
Comment on lines 23 to 43
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));
}
Copy link
Collaborator

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 {
Copy link
Collaborator

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 Show resolved Hide resolved
engine/src/core/error/expected.h Outdated Show resolved Hide resolved
engine/src/core/error/expected.h Outdated Show resolved Hide resolved
else new(&invalid) U(std::move(rhs.invalid));
}

constexpr expected<E, U>& operator=(const expected<E, U>& other) {
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Comment on lines 65 to 67
[[nodiscard]] constexpr bool has_value() const noexcept { return _isOk; }

[[nodiscard]] constexpr bool isOk() const noexcept { return _isOk; }
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants