Skip to content

[variant] Handle CV types as arguments#20

Merged
Tsche merged 1 commit intomasterfrom
feature/cv_types_for_variant
Oct 15, 2025
Merged

[variant] Handle CV types as arguments#20
Tsche merged 1 commit intomasterfrom
feature/cv_types_for_variant

Conversation

@Yaraslaut
Copy link
Copy Markdown
Member

Closes #19

Will make it a draft since requires rebase on top of #18

@Yaraslaut Yaraslaut requested review from Tsche and Copilot and removed request for Tsche September 29, 2025 18:16
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the variant implementation to handle CV-qualified types as arguments to variant operations. The primary goal is to allow the variant to work correctly with const/volatile qualified types in both storage and access operations.

Key Changes

  • Enhanced variant template argument handling to support CV-qualified types
  • Added comprehensive test coverage for CV-qualified variants and reference-based get operations
  • Improved type deduction for variant assignment operations

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
include/rsl/variant Core variant implementation updates for CV type handling, assignment operators, and storage generation
test/variant/variant.get/get.cpp Comprehensive test implementation for variant get operations with CV-qualified types
test/variant/variant.assign/assign.cpp New test file for variant assignment operations
test/variant/variant.ctor/default.cpp Additional constructor tests for CV-qualified variants
Multiple CMakeLists.txt Build configuration updates to include new test files

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Yaraslaut Yaraslaut force-pushed the feature/cv_types_for_variant branch from 9de884d to 29ba90e Compare October 7, 2025 07:12
@Yaraslaut Yaraslaut marked this pull request as ready for review October 7, 2025 07:12
Copy link
Copy Markdown
Member

@Tsche Tsche left a comment

Choose a reason for hiding this comment

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

other than that lgtm

define_aggregate(^^Storage,
{data_member_spec(^^char, {.name="_rsl_dummy"}),
data_member_spec(^^Ts, {.name= "_rsl_alt" + util::to_string(idx++)})...
data_member_spec(std::meta::remove_cv(^^Ts), {.name= "_rsl_alt" + util::to_string(idx++)})...
Copy link
Copy Markdown
Member

@Tsche Tsche Oct 8, 2025

Choose a reason for hiding this comment

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

do not spell out std::meta:: here. remove_cv can be found via ADL - the argument is of type std::meta::info. Please apply this change to tagged_variant::make_storage as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also I can't help but wonder if remove_cvref is better here. Union members cannot be references - so variant<T&> is rejected either way. Might be worth it to make extra sure it cannot possibly fail at this point.

@Yaraslaut Yaraslaut force-pushed the feature/cv_types_for_variant branch from 29ba90e to 774efb1 Compare October 9, 2025 12:46
@Tsche Tsche merged commit afc59a0 into master Oct 15, 2025
@Tsche Tsche deleted the feature/cv_types_for_variant branch October 29, 2025 02:51
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.

rsl::variant allowed template arguments

3 participants