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

Address #1138 when mixing holder_type's upon casting an object. #1139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EricCousineau-TRI
Copy link
Collaborator

This presently fails test_move_only_holder. Will inspect if there is a way around this - perhaps only killing the existing holder if there is no type match between holders?

This addresses #1138.

@EricCousineau-TRI
Copy link
Collaborator Author

Updated to pass tests, simplified override for holder_helper to stay in the same class.

@jagerman
Copy link
Member

The core issue in #1138 is that we don't check the holder type at all when initializing an instance: we just assume that a function returning a holder is returning the right kind of holder. I've just submitted #1161 to detect that and fail if it happens when attempting to register the type and/or function.

There are some other cases this PR doesn't catch that the other one does: for example returning a std::shared_ptr<A> that needs a std::unique_ptr<A>, or one that returns a boost::shared_ptr<A> to a type needing an std::shared_ptr<A>, or various other combinations.

More problematic, the change here would also break backwards-compatibility (and indeed breaks the current test suite) by requiring that any move-only holder adds a release() method.

Conversion between holder types--e.g. being able to convert an rvalue unique_ptr into a shared_ptr seems feasible, but I think it's going to need a different path than stealing the pointer and stashing it in the new type. std::shared_ptr has a std::unique_ptr && constructor for doing exactly this promotion, but we'd need a nice way to detect that case, and ideally, detect it at compile time rather than run-time.

@EricCousineau-TRI
Copy link
Collaborator Author

Sweet! Will close this PR shortly.

Will comment on the other PR.

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Oct 29, 2017

the change here would also break backwards-compatibility

Philosophical question here: Can I ask what the actual intent is in delineating holder between copyable and move-only, rather than keeping it more towards shared and unique?
Can I ask if there are existing usages in external code bases, where move-only != unique (at an abstract level) and copyable != shared?

@jagerman
Copy link
Member

Philosophical question here: Can I ask what the actual intent is in delineating holder between copyable and move-only, rather than keeping it more towards shared and unique?

It's not really a philosophical question, it's just that we have fundamentally different code paths when a holder can be copied, with the copyable_ and move_only_ base classes capturing the different implementations. Essentially, we are already basically assuming that copyable means shared ownership and move-only implies unique ownership.

But more generally (and the thing that affects backwards compatibility) is that we allow struct holder_helper specializations to be used to adapt any random smart pointer type that someone has on hand, but that such a specialization isn't needed if the holder type has a get() method that returns the pointer.

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