Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

variant varadic default constructed state #804

Conversation

fermentedfly
Copy link
Contributor

Hi,

It seems variant_varadic still contains valid type_id when default constructed,
initializing to variant_npos should resolve the issue.

Regards,
Manuel

Copy link

Review changes with SemanticDiff.

@jwellbelove jwellbelove changed the base branch from master to pull-request/#804-variant-varadic-default-constructed-state February 22, 2024 19:14
@jwellbelove jwellbelove merged commit 134f8e4 into ETLCPP:pull-request/#804-variant-varadic-default-constructed-state Feb 22, 2024
63 checks passed
@fermentedfly fermentedfly deleted the fix_variant_default_contructed_state branch February 23, 2024 19:54
jwellbelove pushed a commit that referenced this pull request Mar 4, 2024
@Chiraffollo
Copy link
Contributor

Hi,
when I upgraded the etl library in my local project to the version 2.38.11 from 2.38.10, I stumbled across this ticket.
For std::variant, it is specified that it contains an element of the first type if default constructed and thus the index must be 0. Is there a reason why the etl must differ here? If so, the creation of the first type would have to be removed in the default constructor and it would also be helpful if a note were added to the documentation that this deviates from std::variant and that the class therefore does not behave as expected.

@jwellbelove
Copy link
Contributor

Is there a reason why the etl must differ here?

You are right, this change does look incorrect.

@Chiraffollo
Copy link
Contributor

Should I create a pull request?

@jwellbelove
Copy link
Contributor

No need, I'm doing a simple revert, plus a couple of extra tests to check the value initialisation.

@Chiraffollo
Copy link
Contributor

Okay, thank you very much! Have a nice weekend.

jwellbelove pushed a commit that referenced this pull request Apr 26, 2024
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.

3 participants