Skip to content

Implement LWG-4072 std::optional comparisons: constrain harder - #6424

Open
A. Jiang (frederick-vs-ja) wants to merge 3 commits into
microsoft:mainfrom
frederick-vs-ja:lwg-4072
Open

Implement LWG-4072 std::optional comparisons: constrain harder#6424
A. Jiang (frederick-vs-ja) wants to merge 3 commits into
microsoft:mainfrom
frederick-vs-ja:lwg-4072

Conversation

@frederick-vs-ja

Copy link
Copy Markdown
Contributor

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.

  • For operators comparing optional and other types, constraints were added in C++17 mode before the initial open-sourcing commit (2195148).
  • For operators comparing two optional values, 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.

  1. First, this PR modifies the _Implicitly_convertible_to concept to handle return types whose prvalues can be converted to bool but xvalues can't. Such classes are implementable due C++23 WG21-P0847R7 "Deducing this" as patched by CWG-2813. This part also affects comparisons for move_iterator and reverse_iterator.
  2. Second, this PR uses the _Enable_meow helper alias templates for operators comparing two optionals instead of require-clauses. This makes the implementation strategy less clear but is necessary for C++17 mode.
  3. Third, this PR introduces the _Enable_self_if_not_optional helper 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_meow helpers.

Test cases for the modification to _Implicitly_convertible_to is currently not enabled for MSVC because MSVC hasn't implemented CWG-2813 yet. See DevCom-10817483.

Copilot AI balanced review requested due to automatic review settings August 30, 2026 15:16
@frederick-vs-ja
A. Jiang (frederick-vs-ja) requested a review from a team as a code owner August 30, 2026 15:16
@github-project-automation github-project-automation Bot moved this to Initial Review in STL Code Reviews Aug 30, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI 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.

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.

Comment thread tests/std/tests/P0220R1_optional/test.cpp Outdated
Comment thread tests/std/tests/P0220R1_optional/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp Outdated
Copilot AI review requested due to automatic review settings August 30, 2026 15:26

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings September 5, 2026 05:51

Copilot AI 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.

🟡 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

Comment thread stl/inc/optional
_Enable_if_bool_convertible<decltype(_STD declval<const _Lhs&>() >= _STD declval<const _Rhs&>())>;

template <class _Ty>
using _Enable_self_if_not_optional = enable_if_t<!_Is_specialization_v<_Ty, optional>, _Ty>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

What about _Remove_cvref_t?

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

Labels

enhancement Something can be improved

Projects

Status: Initial Review

Development

Successfully merging this pull request may close these issues.

4 participants