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

Improve move semantics in Expected #4326

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented Oct 19, 2022

This patch unconditionally moves an Unexpected<U> value parameter as long as U is not a reference. If U is a reference the code should not compile. An error type that holds a reference is a strange use-case, and an overload is not provided. If it is required in the future it can be added.

The Expected(U r) overload should take a forwarding ref.

This patch unconditionally moves an `Unexpected<U>` value parameter as
long as `U` is not a reference. If `U` is a reference the code should
not compile. An error type that holds a reference is a strange use-case,
and an overload is not provided. If it is required in the future it can
be added.

The `Expected(U r)` overload should take a forwarding ref.
@seelabs
Copy link
Collaborator Author

seelabs commented Oct 19, 2022

I'm a little tempted to replace the enable_if with concepts in this patch too, but decided that would hide the "real" change. Concepts can come later.

@seelabs
Copy link
Collaborator Author

seelabs commented Oct 19, 2022

I did add a patch to replace enable_if with concepts. I'm fine removing it if we want to keep the enable_if for now and refactor with concepts later.

Edit: Ugh, I just noticed how our automatic formatting screws that up. We might have to update our clang-format before we start using concepts.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for fixing my bugs!

typename = std::enable_if_t<std::is_convertible_v<U, T>>>
constexpr Expected(U r) : Base(T{std::forward<U>(r)})
template <typename U>
requires std::convertible_to<U, T> constexpr Expected(U && r)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. We really need to tune up clang-format for requires clauses. I think this would read way better as...

    template <typename U>
    requires std::convertible_to<U, T>
    constexpr Expected(U&& r)
        : Base(T{std::forward<U>(r)})
    {
    }

But we need to get a few test cases into the code base before we clean that up. Carry on...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we use clang 10 for formatting. When I use clang 15 is formats in a reasonable way. I hope we can update our clang format shortly. I also don't mind delaying this patch until after we update formatting.

@seelabs seelabs added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 10, 2022
@intelliot intelliot changed the base branch from develop to releases/1.10.0-rc1 November 28, 2022 21:59
@intelliot intelliot merged commit 518fb6d into XRPLF:releases/1.10.0-rc1 Nov 28, 2022
dangell7 pushed a commit to Transia-RnD/rippled that referenced this pull request Mar 5, 2023
* Improve move semantics in Expected:

This patch unconditionally moves an `Unexpected<U>` value parameter as
long as `U` is not a reference. If `U` is a reference the code should
not compile. An error type that holds a reference is a strange use-case,
and an overload is not provided. If it is required in the future it can
be added.

The `Expected(U r)` overload should take a forwarding ref.

* Replace enable_if with concepts in Expected
tequdev pushed a commit to tequdev/rippled that referenced this pull request Nov 17, 2023
* Improve move semantics in Expected:

This patch unconditionally moves an `Unexpected<U>` value parameter as
long as `U` is not a reference. If `U` is a reference the code should
not compile. An error type that holds a reference is a strange use-case,
and an overload is not provided. If it is required in the future it can
be added.

The `Expected(U r)` overload should take a forwarding ref.

* Replace enable_if with concepts in Expected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants