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

Improve custom holder support #607

Merged
merged 2 commits into from
Jan 31, 2017
Merged

Improve custom holder support #607

merged 2 commits into from
Jan 31, 2017

Conversation

dean0x7d
Copy link
Member

@dean0x7d dean0x7d commented Jan 18, 2017

This PR resolves issues #585 and #605.

  1. Adds a helper struct to abstract away the holder's .get() function as suggested by @aldanor in Working with a homebrewed shared pointer type that does not implement .get() #585. This is only required for type's with a renamed .get().

    An alternative solution would be to use just a function instead of the struct, but in that case, the user-provided overload would need to be placed either in the global namespace or in the same namespace as the custom holder (ADL rules) which could possibly conflict with existing functions.

  2. Support for custom move-only holders. It seems that so far only std::unique_ptr worked properly. For move-only types init_holder_helper would create a new holder from a raw pointer. This works for std::unique_ptr because its type_caster specialization would call .release(). However, custom move-only holders were handled using type_caster_holder which does not call .release() leading to the issue reported in Custom owning pointer #605.

    This PR renames type_caster_holder to copyable_holder_caster and adds a move_only_holder_caster. The type_caster<std::unique_ptr<type, deleter>> is removed in favor of move_only_holder_caster which does the same job but accepts any move-only holder. Instead of calling .release() on the moved-out pointer, this is now handled by the move constructor in init_holder_helper. This is a more portable solution since the custom holder type might not have a .release() function.

    An alternative solution would be to handle all holders using one type_caster_holder, but this would require some ugly SFINAE to disable .load() for move-only holders and for cast() to accept copyable holders only by const holder_type & and move-only by holder_type &&.

@dean0x7d dean0x7d mentioned this pull request Jan 18, 2017
@@ -206,6 +214,18 @@ struct SharedFromThisRef {
std::shared_ptr<B> shared = std::make_shared<B>();
};

template <typename T>
class UniquePtr {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would give this a different name to make explicit that one doesn't want to do this for std::unique_ptr. E.g. CustomUniquePtr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll rename it.

static void init_holder_from_existing(instance_type *inst, const holder_type *holder_ptr,
std::false_type /*is_copy_constructible*/) {
new (&inst->holder) holder_type(std::move(*const_cast<holder_type *>(holder_ptr)));
}
Copy link
Contributor

@pschella pschella Jan 18, 2017

Choose a reason for hiding this comment

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

Perhaps add a two argument version to dispatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. Two argument version of what?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean adding static void init_holder_from_existing(instance_type *inst, const holder_type *holder_ptr) to dispatch between the two options.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think another layer of abstraction is needed here since this is all implementation detail and the intent should be clear from init_holder_helper right below.

@dean0x7d dean0x7d mentioned this pull request Jan 19, 2017
@dean0x7d
Copy link
Member Author

This PR also fixes #608. The issue there was that type_caster<std::unique_ptr<type, deleter>>::cast() got return_value_policy::move but the only viable policy when casting a holder is take_ownership. The new move_only_holder_caster::cast() applies the correct policy unconditionally.

Custom holder types which don't have `.get()` can select the correct
function to call by specializing `holder_traits`.
@wjakob
Copy link
Member

wjakob commented Jan 31, 2017

This looks great, thank you!

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.

None yet

4 participants