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

Cleanup up handling of references. #70

Closed
wants to merge 2 commits into from
Closed

Cleanup up handling of references. #70

wants to merge 2 commits into from

Conversation

joto
Copy link
Contributor

@joto joto commented Jan 26, 2016

Storing references hasn't worked before. This commit documents the fact, adds a
static check for it and tests this case. We also test the recommended
alternative, which is to use std::reference_wrapper.

Storing references hasn't worked before. This commit documents the fact, adds a
static check for it and tests this case. We also test the recommended
alternative, which is to use std::reference_wrapper.
@springmeyer
Copy link
Contributor

👍 This looks good.

@lightmare
Copy link
Contributor

I think the last diff hunk is wrong. As is explained in the link in the comment above it: since T is a deduced type, T&& is a universal/forwarding reference, it can bind to rvalue reference or lvalue reference. T may be deduced as X const& for some type X, that's why remove_reference is needed -- because (this implementation) variant can only store X, not X const&.

@artemp
Copy link
Contributor

artemp commented Jan 26, 2016

@joto - @lightmare is right and that was my motivation: to allow T being deduced either to rvalue (move) or lvalue (copy). I'll take a fresh look tomorrow again but the original code looks correct to me.

@lightmare
Copy link
Contributor

A concrete example for illustration:

int a = 5;
variant<int> v(a);

The conversion constructor will be variant::variant(T && x) with T == int&. remove_reference<T> == int, that's what you want to store.

@joto
Copy link
Contributor Author

joto commented Jan 27, 2016

As per chat with @artemp I committed the changes to master without the wrong removal of std::remove_reference and close this pull request now.

@joto joto closed this Jan 27, 2016
@joto joto deleted the forbid_references branch January 27, 2016 16:04
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.

4 participants