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

Support to-object conversions for std::reference_wrapper<const T>. #384

Merged
merged 1 commit into from
Nov 13, 2015

Conversation

jpetso
Copy link
Contributor

@jpetso jpetso commented Nov 11, 2015

Previously the conversion would fail because struct object is not
generally provided for the const version of the type, but because
the wrapper would pass down the type unchanged, it would look for
exactly that missing template specialization unsuccessfully.

This is specifically an issue for std::reference_wrapper because
std::cref() returns an std::refererence_wrapper.

I haven't got GTest set up on my system, hopefully the buildbot runs the tests. If not, please run manually.

Note: convert<>() passes through the same template but it makes little sense to convert from object to const variable, because the variable is const. So I left that part unchanged.

Previously the conversion would fail because struct object is not
generally provided for the const version of the type, but because
the wrapper would pass down the type unchanged, it would look for
exactly that missing template specialization unsuccessfully.

This is specifically an issue for std::reference_wrapper because
std::cref() returns an std::reference_wrapper<const T>.
@redboltz
Copy link
Contributor

@jpetso, thank you for sending the PR. It looks good to me. I will merge it.

By the way, I'd like to ask you a question about github. As far as I can remember, you sent the first version of the PR. But it had a compile error. Now, no compile errors are observed. Did you update the PR? I sometimes meet the same situation. I know two ways to fix the problem. One is committing the fix and push it. The other is remove the branch from my github (at that time PR is automatically closed), and then, push the fixed-branch and create the new PR. The problem of the former approach is many commits. I'd like to squash it. The problem of latter approach is creating an additional PR. However, it seems that you are doing well. How do you update your PR? If I amend my commit and push it, it makes conflict because some part of commit is already applied.

@jpetso
Copy link
Contributor Author

jpetso commented Nov 12, 2015

I force-pushed to my branch with a different (amended) commit after I saw the error, which automatically (silently) updates the PR and re-triggers the build. Force-pushing (git push <remote> <branch> -f) overrides the safeguard that would usually keep you from replacing already-pushed commits.

@redboltz
Copy link
Contributor

Thank you @jpetso, good to know.

redboltz added a commit that referenced this pull request Nov 13, 2015
Support to-object conversions for std::reference_wrapper<const T>.
@redboltz redboltz merged commit b76c8ae into msgpack:master Nov 13, 2015
@jpetso jpetso deleted the jpetso_refwrap branch November 13, 2015 18:54
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.

2 participants