Fix expected: operator!= SFINAE, [[nodiscard]], constexpr, and add tests - #44
Conversation
…SCARD - operator!= for expected<T1,E1> vs expected<T2,E2> and the void specialization checked != convertibility in SFINAE but the body uses !(lhs == rhs), so the constraint should check == convertibility instead. This fixes a conformance issue for types that define == but not !=. - Replace all raw [[nodiscard]] with YK_POLYFILL_NODISCARD for C++11 compatibility, matching the project's feature-gating convention. - Remove unused times_two helper in test/cxx11/expected.cpp. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199s51dz1Jety9DKdnA9JRY
…orm, monadic rvalue overloads - Test value()/error()/operator*() rvalue overloads for correct reference types and actual value forwarding (both non-void and void expected) - Test value() rvalue throwing bad_expected_access with the moved error - Test transform with a function pointer (times_two) - Test and_then/or_else/transform/transform_error rvalue overloads Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199s51dz1Jety9DKdnA9JRY
The const&& overloads are const member functions, so they are valid constexpr in C++11. They were incorrectly marked YK_POLYFILL_CXX14_CONSTEXPR, creating a mismatch with their constexpr callers (operator*() const&&, error() const&&). Change them to plain constexpr. The non-const && overloads correctly remain YK_POLYFILL_CXX14_CONSTEXPR since non-const constexpr member functions require C++14. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199s51dz1Jety9DKdnA9JRY
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the expected polyfill by replacing standard [[nodiscard]] attributes with YK_POLYFILL_NODISCARD, correcting type trait checks in operator!= overloads, and refining constexpr specifiers for const rvalue accessors. It also adds comprehensive unit tests for rvalue observers and monadic operations. The reviewer suggests casting *this directly to base_type const&& instead of using std::move on a base_type const& cast in the const rvalue accessors. This avoids using std::move, which is not guaranteed to be constexpr in C++11, thereby ensuring strict C++11 compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // rvalue accessors on the storage (private base), reached through explicit std::move. | ||
| YK_POLYFILL_CXX14_CONSTEXPR T&& base_get_value() && noexcept { return std::move(static_cast<base_type&>(*this).get_value()); } | ||
| YK_POLYFILL_CXX14_CONSTEXPR T const&& base_get_value() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_value()); } | ||
| constexpr T const&& base_get_value() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_value()); } |
There was a problem hiding this comment.
Instead of casting *this to base_type const& and then calling std::move on the result, you can cast *this directly to base_type const&&. This will invoke the rvalue overload get_value() const&& on the base class, which already returns T const&& directly.
This is cleaner and ensures strict C++11 constexpr compatibility, as std::move is not standard-mandated to be constexpr in C++11.
constexpr T const&& base_get_value() const&& noexcept { return static_cast<base_type const&&>(*this).get_value(); }| constexpr T const&& base_get_value() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_value()); } | ||
| YK_POLYFILL_CXX14_CONSTEXPR E&& base_get_error() && noexcept { return std::move(static_cast<base_type&>(*this).get_error()); } | ||
| YK_POLYFILL_CXX14_CONSTEXPR E const&& base_get_error() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_error()); } | ||
| constexpr E const&& base_get_error() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_error()); } |
There was a problem hiding this comment.
Instead of casting *this to base_type const& and then calling std::move on the result, you can cast *this directly to base_type const&&. This will invoke the rvalue overload get_error() const&& on the base class, which already returns E const&& directly.
This is cleaner and ensures strict C++11 constexpr compatibility, as std::move is not standard-mandated to be constexpr in C++11.
constexpr E const&& base_get_error() const&& noexcept { return static_cast<base_type const&&>(*this).get_error(); }| private: | ||
| YK_POLYFILL_CXX14_CONSTEXPR E&& base_get_error() && noexcept { return std::move(static_cast<base_type&>(*this).get_error()); } | ||
| YK_POLYFILL_CXX14_CONSTEXPR E const&& base_get_error() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_error()); } | ||
| constexpr E const&& base_get_error() const&& noexcept { return std::move(static_cast<base_type const&>(*this).get_error()); } |
There was a problem hiding this comment.
Instead of casting *this to base_type const& and then calling std::move on the result, you can cast *this directly to base_type const&&. This will invoke the rvalue overload get_error() const&& on the base class, which already returns E const&& directly.
This is cleaner and ensures strict C++11 constexpr compatibility, as std::move is not standard-mandated to be constexpr in C++11.
constexpr E const&& base_get_error() const&& noexcept { return static_cast<base_type const&&>(*this).get_error(); }Cast *this directly to base_type const&& (or base_type&&) to invoke the rvalue-qualified get_value()/get_error() overloads, instead of calling the lvalue overload and then std::move on the result. Cleaner and avoids reliance on std::move being constexpr in C++11. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199s51dz1Jety9DKdnA9JRY
2 participants
Summary
Review fixes for the
expectedimplementation in #43.operator!=SFINAE: The twooperator!=overloads (expected<T1,E1>vsexpected<T2,E2>and thevoidspecialization) checked!=convertibility in their SFINAE constraints, but the body uses!(lhs == rhs). Changed to check==convertibility, fixing a conformance issue for types that define==but not!=.[[nodiscard]]withYK_POLYFILL_NODISCARD: All 40 raw[[nodiscard]]usages replaced with the project's portability macro for C++11 compatibility.constexprmismatch onconst&&helpers:base_get_value() const&&andbase_get_error() const&&were markedYK_POLYFILL_CXX14_CONSTEXPRbut called fromconstexprcallers (operator*() const&&,error() const&&). Sinceconstmember functions are validconstexprin C++11, changed them to plainconstexpr.times_twotest helper.value()/error()/operator*(),transformwith a function pointer, and monadic rvalue overloads (and_then/or_else/transform/transform_erroron&&).Test plan
Generated by Claude Code