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

WIP Add arg().disown() (superseding the consume call_policy) #1226

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,11 @@ struct argument_record {
const char *descr; ///< Human-readable version of the argument value
handle value; ///< Associated Python object
bool convert : 1; ///< True if the argument is allowed to convert when loading
bool disown : 1; ///< TODO docs
bool none : 1; ///< True if None is allowed when loading

argument_record(const char *name, const char *descr, handle value, bool convert, bool none)
: name(name), descr(descr), value(value), convert(convert), none(none) { }
argument_record(const char *name, const char *descr, handle value, bool convert, bool disown, bool none)
: name(name), descr(descr), value(value), convert(convert), disown(disown), none(none) { }
};

/// Internal data structure which holds metadata about a bound function (signature, overloads, etc.)
Expand Down Expand Up @@ -353,16 +354,16 @@ template <> struct process_attribute<is_new_style_constructor> : process_attribu
template <> struct process_attribute<arg> : process_attribute_default<arg> {
static void init(const arg &a, function_record *r) {
if (r->is_method && r->args.empty())
r->args.emplace_back("self", nullptr, handle(), true /*convert*/, false /*none not allowed*/);
r->args.emplace_back(a.name, nullptr, handle(), !a.flag_noconvert, a.flag_none);
r->args.emplace_back("self", nullptr, handle(), true /*convert*/, false /*disown*/, false /*none not allowed*/);
r->args.emplace_back(a.name, nullptr, handle(), !a.flag_noconvert, a.flag_disown, a.flag_none);
}
};

/// Process a keyword argument attribute (*with* a default value)
template <> struct process_attribute<arg_v> : process_attribute_default<arg_v> {
static void init(const arg_v &a, function_record *r) {
if (r->is_method && r->args.empty())
r->args.emplace_back("self", nullptr /*descr*/, handle() /*parent*/, true /*convert*/, false /*none not allowed*/);
r->args.emplace_back("self", nullptr /*descr*/, handle() /*parent*/, true /*convert*/, false /*disown*/, false /*none not allowed*/);

if (!a.value) {
#if !defined(NDEBUG)
Expand All @@ -385,7 +386,7 @@ template <> struct process_attribute<arg_v> : process_attribute_default<arg_v> {
"Compile in debug mode for more information.");
#endif
}
r->args.emplace_back(a.name, a.descr, a.value.inc_ref(), !a.flag_noconvert, a.flag_none);
r->args.emplace_back(a.name, a.descr, a.value.inc_ref(), !a.flag_noconvert, a.flag_disown, a.flag_none);
}
};

Expand Down
9 changes: 8 additions & 1 deletion include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,7 @@ template <typename... Ts> class type_caster<std::tuple<Ts...>>
template <typename T>
struct holder_helper {
static auto get(const T &p) -> decltype(p.get()) { return p.get(); }
static void release(T &p) { p.release(); }
};

/// Type caster for holder types like std::shared_ptr, etc.
Expand Down Expand Up @@ -1714,16 +1715,19 @@ template <return_value_policy policy = return_value_policy::automatic_reference,
/// Annotation for arguments
struct arg {
/// Constructs an argument with the name of the argument; if null or omitted, this is a positional argument.
constexpr explicit arg(const char *name = nullptr) : name(name), flag_noconvert(false), flag_none(true) { }
constexpr explicit arg(const char *name = nullptr) : name(name), flag_noconvert(false), flag_disown(false), flag_none(true) { }
/// Assign a value to this argument
template <typename T> arg_v operator=(T &&value) const;
/// Indicate that the type should not be converted in the type caster
arg &noconvert(bool flag = true) { flag_noconvert = flag; return *this; }
/// TODO: docs
arg &disown(bool flag = true) { flag_disown = flag; return *this; }
/// Indicates that the argument should/shouldn't allow None (e.g. for nullable pointer args)
arg &none(bool flag = true) { flag_none = flag; return *this; }

const char *name; ///< If non-null, this is a named kwargs argument
bool flag_noconvert : 1; ///< If set, do not allow conversion (requires a supporting type caster!)
bool flag_disown : 1; ///< TODO: docs
bool flag_none : 1; ///< If set (the default), allow None to be passed to this argument
};

Expand Down Expand Up @@ -1757,6 +1761,9 @@ struct arg_v : arg {
/// Same as `arg::noconvert()`, but returns *this as arg_v&, not arg&
arg_v &noconvert(bool flag = true) { arg::noconvert(flag); return *this; }

/// Same as `arg::disown()`, but returns *this as arg_v&, not arg&
arg_v &disown(bool flag = true) { arg::disown(flag); return *this; }

/// Same as `arg::nonone()`, but returns *this as arg_v&, not arg&
arg_v &none(bool flag = true) { arg::none(flag); return *this; }

Expand Down
33 changes: 33 additions & 0 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,39 @@ class cpp_function : public function {
call.args_convert.swap(second_pass_convert);
}


//printf("calling %s %s, args %d %lu\n", func.name, func.signature, func.nargs, func.args.size());
for (size_t i = 0; i < func.args.size(); ++i) {
if (!func.args[i].disown) {
//printf("NOT disowning arg %lu\n", i);
continue;
} else {
//printf("DISOWNING arg %lu\n", i);
}

auto &arg = call.args[i];
if (!arg)
pybind11_fail("nothing to disown?!");

if (arg.is_none())
continue; /* Nothing to disown */

auto inst = reinterpret_cast<detail::instance *>(arg.ptr());
auto value_and_holder = values_and_holders(inst).begin();


using holder_type = std::unique_ptr<void*>; // how do I get the proper one?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@torokati44 After seeing your stuff, and reworking my PR, I believe this function should be what you need for type-erased holder transfer:
https://github.com/EricCousineau-TRI/pybind11/blob/379afa73c7fedded57e4fedc9c3b077cb7292643/include/pybind11/cast.h#L482

You could use it to something like the tune of:

if (!reclaim_existing_if_needed(inst, tinfo, existing_holder)) {
  throw cast_error("Cannot release ownership of this object");  // If you have an incompatible holder type, like `shared_ptr`.
}

The caveat is, you'll need to figure out what the holder type is for the given argument. I believe that should be elsewhere in this area of code.
(Also, ensure that you get tinfo from the argument type and NOT the holder, as holder->type may be null!)


auto &holder = value_and_holder->holder< holder_type >();


holder_helper<holder_type>::release(holder);

value_and_holder->set_holder_constructed(false);
inst->owned = false;
holder.~holder_type();
}

// 6. Call the function.
try {
loader_life_support guard{};
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ set(PYBIND11_TEST_FILES
test_enum.cpp
test_eval.cpp
test_exceptions.cpp
test_disown.cpp
test_factory_constructors.cpp
test_iostream.cpp
test_kwargs_and_defaults.cpp
Expand Down
50 changes: 50 additions & 0 deletions tests/test_disown.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
tests/test_disown.cpp -- disown

Copyright (c) 2016 Wenzel Jakob <[email protected]>
Copyright (c) 2017 Attila Török <[email protected]>

All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/

#include "pybind11_tests.h"

class Box {
int size;
static int num_boxes;

public:
Box(int size): size(size) { py::print("Box created."); ++num_boxes; }
~Box() { py::print("Box destroyed."); --num_boxes; }

int get_size() { return size; }
static int get_num_boxes() { return num_boxes; }
};

int Box::num_boxes = 0;

class Filter {
int threshold;

public:
Filter(int threshold): threshold(threshold) { py::print("Filter created."); }
~Filter() { py::print("Filter destroyed."); }

void process(Box *box) { // ownership of box is taken
py::print("Box is processed by Filter.");
if (box->get_size() > threshold)
delete box;
// otherwise the box is leaked
};
};

test_initializer disown([](py::module &m) {
py::class_<Box>(m, "Box")
.def(py::init<int>())
.def_static("get_num_boxes", &Box::get_num_boxes);

py::class_<Filter>(m, "Filter")
.def(py::init<int>())
.def("process", &Filter::process, py::arg("box").disown());
});
42 changes: 42 additions & 0 deletions tests/test_disown.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import pytest


def test_disown_argument(capture):
from pybind11_tests import Box, Filter

with capture:
filt = Filter(4)
assert capture == "Filter created."
with capture:
box_1 = Box(1)
box_8 = Box(8)
assert capture == """
Box created.
Box created.
"""

assert Box.get_num_boxes() == 2

with capture:
filt.process(box_1) # box_1 is not big enough, but process() leaks it
assert capture == "Box is processed by Filter."

assert Box.get_num_boxes() == 2

with capture:
filt.process(box_8) # box_8 is destroyed by process() of filt
assert capture == """
Box is processed by Filter.
Box destroyed.
"""

assert Box.get_num_boxes() == 1 # box_1 still exists somehow, but we can't access it

with capture:
del filt
del box_1
del box_8
pytest.gc_collect()
assert capture == "Filter destroyed."

assert Box.get_num_boxes() == 1 # 1 box is leaked, and we can't do anything