Skip to content

Adding tests for LWG-4025 #28

Merged
X1aomu merged 2 commits into
zeus-cpp:mainfrom
ComixHe:main
Jul 2, 2026
Merged

Adding tests for LWG-4025 #28
X1aomu merged 2 commits into
zeus-cpp:mainfrom
ComixHe:main

Conversation

@ComixHe

@ComixHe ComixHe commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Implements LWG-4025 by omitting operator=(move_assign_base&&) for expected<void, E> when E is not movable, letting the defaulted move assignment fall back to copy.

This produces identical trait results to a conforming C++23 implementation, but is a structural deviation: the operator exists (dispatching to copy) where the standard excludes it, detectable only via custom SFINAE.

The alternative (SFINAE-constraining the operator) would break triviality (LWG-4026) for the common case. Would this approach be acceptable?

@X1aomu X1aomu self-assigned this Jun 30, 2026
@X1aomu

X1aomu commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

I have some good news: we can actually achieve both—implementing LWG-4026 without breaking the custom SFINAE scenario you mentioned.
And perhaps even better news: the current implementation already satisfies these conditions without requiring any modifications. 🤣

To verify this, we can simply keep the test additions (though the custom-SFINAE test case will need a minor adjustment). There is no need to modify the implementation itself; running the tests directly will confirm this. :)

I appreciate the tests you've provided. Although the core implementation doesn't need to be updated, these test cases remain valuable for future regression testing, and I would very much like to keep them in the repository.

As a supplementary note regarding my understanding of LWG-4025: the reason the standard's resolution only targets expected<void, E> is that only the void specification had an issue. In zeus::expected, our implementation actually already satisfies the logical requirements of LWG-4025. Because we are targeting C++17 and cannot rely on C++20 concepts, we had to use an alternative implementation technique that applies uniformly to both expected<T, E> and expected<void, E>. Therefore, if we ever needed to make explicit code changes based on the LWG-4025 resolution, we couldn't just blindly patch expected<void, E> to match the standard's wording. We would need to make broader, symmetrical changes across both variants to maintain our internal consistency.

With this symmetry in mind, it would be even better if the test cases could be expanded to cover both expected<void, E> and expected<T, E>.

Thanks again for the contribution and the constructive discussion.

@X1aomu X1aomu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I‘d like to keep only the tests. And it would be better (not required) if tests could cover expected<T, E>.

@ComixHe ComixHe force-pushed the main branch 3 times, most recently from 87bc97a to 973dd20 Compare July 2, 2026 04:16
@ComixHe

ComixHe commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review and the clarification. I cross-checked the standard and confirmed the existing implementation is correct.

Updated the PR accordingly — reverted the code change, added the standard rationale in README, and expanded tests to cover expected<T, E> as suggested.

@ComixHe ComixHe requested a review from X1aomu July 2, 2026 05:34

@X1aomu X1aomu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates! The README is a bit verbose here. Maybe we can just put those details in the tests.

@ComixHe ComixHe requested a review from X1aomu July 2, 2026 07:31
@ComixHe ComixHe changed the title Implement LWG-4025 Adding tests for LWG-4025 Jul 2, 2026
@X1aomu X1aomu merged commit c0ff8ca into zeus-cpp:main Jul 2, 2026
26 checks passed
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