Implement LWG-4072 std::optional comparisons: constrain harder - #6424
Implement LWG-4072 std::optional comparisons: constrain harder#6424A. Jiang (frederick-vs-ja) wants to merge 3 commits into
std::optional comparisons: constrain harder#6424Conversation
There was a problem hiding this comment.
Pull request overview
Implements stricter, downlevel constraints for std::optional comparisons per LWG-4072 and improves implicit-conversion detection.
Changes:
- Constrains optional-to-optional and heterogeneous comparisons.
- Handles prvalue-only conversions to
bool. - Adds optional and iterator comparison tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
stl/inc/concepts |
Refines implicit-conversion detection. |
stl/inc/optional |
Adds stricter comparison constraints. |
stl/inc/yvals_core.h |
Records the partial P2944R3 implementation. |
tests/std/tests/P0220R1_optional/test.cpp |
Tests optional comparison constraints. |
tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp |
Tests affected iterator comparisons. |
Suppressed comments (5)
tests/std/tests/P0220R1_optional/test.cpp:8995
- The
<detector validates the return type of==, leaving the return type of the detected<expression unchecked.
std::enable_if_t<std::is_same_v<decltype(std::declval<T>() == std::declval<U>()), bool>, bool>{true};
tests/std/tests/P0220R1_optional/test.cpp:9007
- The
>detector validates the return type of==, leaving the return type of the detected>expression unchecked.
std::enable_if_t<std::is_same_v<decltype(std::declval<T>() == std::declval<U>()), bool>, bool>{true};
tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp:3567
- Correct the misspelled transition marker.
#if _HAS_CXX23 && (defined(__clang__) || defined(__EDG__)) // TRANSTION, DevCom-10817483 (CWG-2813)
tests/std/tests/P0220R1_optional/test.cpp:9001
- The
<=detector validates the return type of==, leaving the return type of the detected<=expression unchecked.
std::enable_if_t<std::is_same_v<decltype(std::declval<T>() == std::declval<U>()), bool>, bool>{true};
tests/std/tests/P0220R1_optional/test.cpp:9013
- The
>=detector validates the return type of==, leaving the return type of the detected>=expression unchecked.
std::enable_if_t<std::is_same_v<decltype(std::declval<T>() == std::declval<U>()), bool>, bool>{true};
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Stephan T. Lavavej (StephanTLavavej)
added
the
enhancement
There was a problem hiding this comment.
🟡 Changes recommended
The optional exclusion fails for cv-qualified optional types, leaving nonconforming heterogeneous overloads enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
I don't think we should do so. The operators comparing two optionals don't accept volatile-qualfied optionals, so changing the constraints to reject volatile-qualified optionals is out of the scope of LWG-4072, and possibly rejects previously valid comparison or change the selected overload.
There was a problem hiding this comment.
What about _Remove_cvref_t?
Towards #4522 and #5272.
Previously, we've been constraining
optional's comparison operators in ways quite similar to WG21-P2944R3, so I believe we've actually partially implemented that paper long time ago.optionaland other types, constraints were added in C++17 mode before the initial open-sourcing commit (2195148).optionalvalues, constraints were added since C++20 mode in Mark several functions as noexcept. #1937.Note that there's currently inconsistency in C++17 mode, which doesn't seem to be a good thing. It's probably better to consistently constrain all these operators either in all modes or only since C++26 mode. As other implementations, especially libc++, constrain them in old modes, I guess we should constrain them in C++17 as indicated by WG21-P2944R3 patched by LWG-4072.
There are three major parts of changes.
_Implicitly_convertible_toconcept to handle return types whose prvalues can be converted toboolbut xvalues can't. Such classes are implementable due C++23 WG21-P0847R7 "Deducing this" as patched by CWG-2813. This part also affects comparisons formove_iteratorandreverse_iterator._Enable_meowhelper alias templates for operators comparing twooptionals instead ofrequire-clauses. This makes the implementation strategy less clear but is necessary for C++17 mode._Enable_self_if_not_optionalhelper alias templates to exactly implement LWG-4072. This approach seems somehow unconventional, but I think it's almost the cleanest way to reusing the existing_Enable_meowhelpers.Test cases for the modification to
_Implicitly_convertible_tois currently not enabled for MSVC because MSVC hasn't implemented CWG-2813 yet. See DevCom-10817483.