Skip to content

fix universal: make utils::expected description consistent#1204

Open
ddvamp wants to merge 2 commits intouserver-framework:developfrom
ddvamp:userver-patch-expected
Open

fix universal: make utils::expected description consistent#1204
ddvamp wants to merge 2 commits intouserver-framework:developfrom
ddvamp:userver-patch-expected

Conversation

@ddvamp
Copy link
Copy Markdown
Contributor

@ddvamp ddvamp commented Apr 25, 2026

Fixes #1203

@ddvamp
Copy link
Copy Markdown
Contributor Author

ddvamp commented Apr 26, 2026 •
edited
Loading

Are you interested in changes/refactoring like those in this commit (7f96faf) ?

Summary of changes:

  • Removed list-initialization that could accidentally invoke an initializer-list constructor instead of the intended copy/move constructor.
  • Made interaction with alternatives (construction, access, e.t.c.) explicit via indices.
  • Removed redundant and incorrect uses of std::forward (assuming that unexpected contains values, not references).
  • Added has_error() method to check for the presence of an error (fixes utils::expected implementation does not take into account valueless_by_exception #1205).
  • Unified the implementations of value() and error() behind a single underlying mechanism.
  • Added a test demonstrating the valueless_by_exception state.

apolukhin
apolukhin requested changes Apr 26, 2026
View reviewed changes
Comment thread universal/include/userver/utils/expected.hpp
template <class S, class E>
expected<S, E>::expected(const S& success)
: data_(success)
: data_(std::in_place_index<0>, success)
Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 26, 2026

Choose a reason for hiding this comment

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

it's easy to accidentally confuse constants 1 and 2. Make those a named constants kSuccessIndex and kErrorIndex and use the readable names everywhere in this file

apolukhin
apolukhin reviewed Apr 26, 2026
View reviewed changes
Comment thread universal/src/utils/expected_test.cpp

EXPECT_FALSE(e.has_value());
EXPECT_THROW(e.value(), utils::bad_expected_access);
EXPECT_FALSE(e.has_error());
Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 26, 2026

Choose a reason for hiding this comment

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

It should behave differently http://eel.is/c++draft/expected.object.assign

Copy link
Copy Markdown
Contributor Author

@ddvamp ddvamp Apr 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Are you suggesting adding an implementation of operator= (copy/move) corresponding to std::expected::operator=?

Copy link
Copy Markdown
Member

@apolukhin apolukhin Apr 28, 2026

Choose a reason for hiding this comment

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

Yep... We are planning to remove utils::expected after a few years and replace it with std::expected

The closer the behavior of those two types - the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apolukhin apolukhin apolukhin requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mistake in description of expected::error()

2 participants

@ddvamp @apolukhin