<variant>: Use a simpler three-way comparator for variant - #6447
Open
A. Jiang (frederick-vs-ja) wants to merge 2 commits into
Open
<variant>: Use a simpler three-way comparator for variant#6447A. Jiang (frederick-vs-ja) wants to merge 2 commits into
<variant>: Use a simpler three-way comparator for variant#6447A. Jiang (frederick-vs-ja) wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Copilot started reviewing on behalf of
A. Jiang (frederick-vs-ja)
September 11, 2026 03:54
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation preserves comparison semantics while avoiding the undesired return-type instantiation.
Pull request overview
Simplifies variant three-way comparison to reduce unnecessary template instantiation and resolves related Clang test failures.
Changes:
- Adds an internal comparator with an explicit return type.
- Updates
variant::operator<=>to use the simpler comparator. - Removes obsolete Clang warning workarounds and expected failures.
File summaries
| File | Description |
|---|---|
stl/inc/variant |
Implements and uses the simplified comparator. |
tests/std/tests/P0088R3_variant/test.cpp |
Removes obsolete diagnostic suppression. |
tests/libcxx/expected_results.txt |
Removes the now-passing libc++ test entry. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot started reviewing on behalf of
A. Jiang (frederick-vs-ja)
September 11, 2026 11:01
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation and test updates are consistent and preserve comparison semantics while reducing instantiation.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
Towards #6156.
Some occurrences of
constexprin the related libcxx test are bogus, and I opened LLVM-222837 to remove them. But it's revealed that MSVC STL's use ofcompare_three_way(using deduced return type in itsoperator()) can cause more things to be instantiated and detected. It seems better to reduce instantiations a bit (at least in the exception specification).This PR introduces a simplified version of
compare_three_wayusing explicit return type, which should be able to stop undesired instantiation and reduce constraint checks.Now LLVM-222837 has been merged, so this PR also mirrors it to MSVC STL's test file. I think it's still valuable to use the new simpler comparator even when there's no bogus
constexprorinline.