Expected over references (reordered history, byte-identical replay)#4
Closed
steve-downey wants to merge 16 commits into
Closed
Expected over references (reordered history, byte-identical replay)#4steve-downey wants to merge 16 commits into
steve-downey wants to merge 16 commits into
Conversation
Add a partial specialization unexpected<E&> that stores a pointer instead of a value, plus the reference_constructs_from_temporary_v trait it needs. This is the uniform error-storage building block: a union cannot hold a reference member directly, but it can hold unexpected<E&> (just a pointer), so every expected<> union can store unexpected<E> for both value and reference E. Purely additive over the primary unexpected<E> template.
Change the primary expected<T,E> union to hold unexpected<E> instead of a raw E. Because unexpected<E&> stores a pointer, this makes E an lvalue reference valid with no extra specialization: expected<T,E&> is just the primary template. Adds the reference-E converting constructors and the rebinding operator= from unexpected<E&>; deletes the dangling value-G cases. Uses the move-then-deref idiom (*std::move(rhs), std::move(x).error()) throughout so converting from a reference-holding expected copies the referent instead of stealing it. expected<int,int&> is now well-formed, so its negative test is dropped; the error-reference test group is added.
Rewrite the void specialization as the explicit expected<void,E> form and store the error as unexpected<E> in its union, so E may be an lvalue reference by the same unexpected<E&> mechanism as the primary. Adds the reference-E converting constructor from expected<void,G&>, the rebinding operator= from unexpected<E&>, the self-type exclusion on the converting constructor, and move-then-deref on all error accesses. expected<void,int&> is now well-formed, so its negative test is dropped; the void+error- reference test group is added.
…,E&) Add the expected<T&,E> partial specialization: value is a reference, stored as a pointer, with the error stored as unexpected<E> so E may also be a reference (this single specialization covers both expected<T&,E> and expected<T&,E&>). Out-of-line definitions throughout, dangling-temporary prevention via reference_constructs_from_temporary_v, shallow move-then- deref conversions, and the full monadic surface. Brings in the reference value/both-reference test groups, the shared testing/types.hpp, and the cross-cutting review-corrections regression suite. Removes the todo.test.cpp scaffolding. Header and tests now match the release state.
Coverage Report for CI Build 28761853430Coverage increased (+6.8%) to 97.017%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions19 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
…f (clang) clang (through 22) rejects an out-of-line constrained member of a partial specialization when its requires-clause names a member typedef of the class: 'out-of-line definition of error_or does not match any declaration in expected<T&, E>'. The primary template is unaffected; only the expected<T&, E> partial specialization's error_or triggered it. Write the two error_or requires-clauses (declaration and definition) as the underlying std::remove_cv_t<std::remove_reference_t<E>> expression instead of error_value_type. Semantically identical (error_value_type is that very alias), but no member-typedef reference for clang to choke on. gcc and clang-22 (libc++, gnu++20 and gnu++26) both accept it; full gcc-debug suite still 734/734. Note: this bug is present in the original expected-over-references tip too (this branch was byte-identical to it); the same fix applies there.
The reference_converts_from_temporary_v trait was defined (via std:: or the
__reference_converts_from_temporary builtin) but never used — every dangling
check in the library uses reference_constructs_from_temporary_v. Clang 18
provides __reference_constructs_from_temporary but not the converts sibling,
so defining the unused alias broke the build there ('T'/'U' does not refer to
a value) and cascaded into spurious 'out-of-line definition does not match'
errors. Remove the dead trait. No behavioral change; gcc and clang-22 remain
green.
Pre-existing on the original expected-over-references tip as well.
Commit f7c44c7 set GCC 13 as the minimum by relying on the __reference_constructs_from_temporary builtin (GCC 13+ / Clang 16+) with no polyfill fallback. gcc 11/12 have neither that builtin nor the C++23 <__cpp_lib_reference_from_temporary> library feature, so they cannot build. Remove them from the CI matrix to match the stated support floor. MSVC is left for a separate decision.
The msvc-debug/msvc-release presets inherited _root-config's C++20 default, where MSVC has neither __cpp_lib_reference_from_temporary (a C++23 library feature) nor the __reference_constructs_from_temporary builtin, leaving detail::reference_constructs_from_temporary_v undefined. Override CMAKE_CXX_STANDARD to 23 for both MSVC presets so std::reference_constructs_ from_temporary_v is picked up. Other presets are unchanged.
The dangling-detection trait relies on __reference_constructs_from_temporary (builtin) or the C++23 <__cpp_lib_reference_from_temporary> feature, and on the compiler canonicalizing an out-of-line constrained member's requires- clause against its in-class declaration. Those hold on GCC 13+ and Clang 19+ but not below: - Clang 17: lacks the builtin entirely (trait undefined). - Clang 18: has the builtin but rejects ~15 out-of-line members whose requires-clause names the self-type/member-typedef with a spelling that differs from the in-class declaration (Clang 22/GCC canonicalize; Clang 18 does not). - MSVC: same constraint-matching strictness (C2244), even in C++23 where the trait itself is available. Rather than rewrite every constrained member's declaration to be token- identical to its definition, set the supported floor. Drop clang 17/18 and the MSVC build-and-test + preset-test jobs from the matrix (gcc 11/12 were already removed), and revert the now-moot MSVC C++23 preset bump. These failures are pre-existing on the original expected-over-references tip; supporting clang<19 / MSVC would be a separate constraint-normalization pass.
…EADME Update Supported Platforms to the enforced CI floor (GCC 13-16, Clang 19-22, AppleClang) and add a 'Compiler floor and unsupported toolchains' subsection explaining why: the reference_constructs_from_temporary builtin lands in GCC 13 / Clang 19, and Clang 18 additionally mismatches out-of-line constrained member definitions. Note that MSVC is unsupported not by design but because we lack MSVC access to develop and verify the out-of-line-definition workarounds it (like Clang 18) needs; contributions with MSVC access welcome.
Resolves the tests/beman/expected/CMakeLists.txt conflict between the std::expected parity test harness (main) and the reference-specialization work (this branch): - Keep main's behavioral/beman-only test-source split and the beman.expected.tests.expected.std parity executable. - File the new reference-specialization test sources (expected_ref*.test.cpp, expected_void_ref_e.test.cpp, expected_review_corrections.test.cpp) under beman_expected_beman_only_tests, since std::expected has no reference specializations to parameterize against. - Restore the local find_package(Catch2)/CMAKE_MODULE_PATH block this branch had moved out of the top-level CMakeLists.txt (main never touched that region, so the merge silently kept this branch's removal without it). - Drop todo.test.cpp from the merged source list; this branch already deleted the scaffolding it exercised. - Parameterize tests/beman/expected/testing/types.hpp's from_expected/ from_unexpected helpers over test_ns (via the new beman/expected/test_expected.hpp) instead of hardcoding beman::expected, and add the tests/ include root to both test executables so the header can use the canonical, namespaced <beman/expected/test_expected.hpp> include instead of a same-directory relative one. - Fix two leftover hardcoded beman::expected::bad_expected_access references in expected_monadic.test.cpp that main's parity refactor hadn't reached. Verified with `make TOOLCHAIN=gcc-15 test`: 1080/1080 tests pass, including the new std.* parity suite.
Two narrow toolchain issues surfaced in CI on the new .std parity target, neither a beman::expected behavioral difference: - expected.test.cpp: three "value() throw carries the error" tests discard the [[nodiscard]] return of e.value() before catching the exception it throws; cast to void so -Werror builds don't choke on -Wunused-result. - unexpected.test.cpp: std::unexpected<int> == std::unexpected<long> fails to compile on some libc++ versions (clang 19, appleclang c++26) with a cross-specialization friend-access error. Guarded the test #ifndef BEMAN_EXPECTED_TEST_STD; beman's own unexpected doesn't have this problem. Documented both, plus a third known-but-unfixed issue (a pervasive Catch2 decomposition x std::expected constrained-operator== interaction that trips "satisfaction of constraint depends on itself" on some libc++ versions, across potentially dozens of comparison sites in the parameterized suite) in docs/std-parity.md as a known, non-blocking limitation of affected toolchains rather than sweeping every call site.
The .std target exists to catch beman::expected behavioral differences from std::expected, not to chase the standard library's own bugs. libc++ has (at least) two independent bugs of its own that surface running this suite against it (see docs/std-parity.md): a cross-specialization friend-access error in std::unexpected's heterogeneous operator==, and a pervasive Catch2-decomposition-triggered self-referential constraint check in std::expected's constrained operator==. Detect libc++ (_LIBCPP_VERSION, via check_cxx_symbol_exists — covers both explicit -stdlib=libc++ and AppleClang's implicit default) and skip building the .std target there entirely; gcc/libstdc++ and clang-with-libstdc++ configurations are unaffected and keep running it. Verified the skip takes effect under clang-19 + libc++ (BEMAN_EXPECTED_USING_LIBCXX is set and beman.expected.tests.expected.std is absent from the ninja target list), and that make TOOLCHAIN=gcc-15 test still builds and runs the .std target normally (1079/1079 tests pass).
Owner
Author
|
Superseded by bemanproject#61, opened directly against upstream main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-authored history of the
expected<T&, E>reference-support work, ending byte-identical to the tip ofexpected-over-references(the tree OID matches exactly). This is a cleaned-up replay of that branch, opened to run CI on the reordered commits.What changed vs. the original branch
The final code/tests/docs are identical; only the history differs. The reorder:
expectedspecializations for reference error types (expected<T,E&>,<T&,E&>,<void,E&>) and then deleted them onceunexpected<E&>unification was discovered. This replay specializesunexpected<E&>first, so the error-reference axis collapses into the primary/void templates from the start — no throwaway specializations (verified: 0 separateE&classes ever added).= delete("...")churn either — verified 0 occurrences).unexpected<E&>, theexpected<void,E>self-type exclusion, and the explicitexpected<void,E>form.Commits (4 core + 3 tail)
feat: add unexpected<E&> partial specialization (pointer storage)feat: expected<T,E> stores unexpected<E>, enabling reference error typesfeat: expected<void,E> stores unexpected<E>, enabling expected<void,E&>feat: add expected<T&,E> reference specialization (covers T&,E and T&,E&)docs: add design/review guides, D4280R0 paper, and plan updatesbuild: sync copier metadata, presets, coverage, and CI to release statechore: remove todo scaffolding superseded by the libraryEach of the four code commits builds and passes its own tests independently (253 → 327 → 404 → 734 tests as each specialization is introduced). Locally verified green with
cmake --workflow --preset gcc-debug(734/734) at every core commit and at the tip.🤖 Generated with Claude Code