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

Add dynamic attribute support #437

Merged
merged 3 commits into from
Oct 14, 2016
Merged

Add dynamic attribute support #437

merged 3 commits into from
Oct 14, 2016

Conversation

dean0x7d
Copy link
Member

@dean0x7d dean0x7d commented Oct 11, 2016

This makes it possible for pybind11 classes to pick up new attributes on the fly, just like regular Python classes. The feature is enabled using py::dynamic_attr:

// C++
py::class_<Foo>("Foo", py::dynamic_attr());
# Python
foo = Foo()
assert not hasattr(foo, 'n')
foo.n = 42
assert hasattr(foo, 'n')

Previous discussions about dynamic attributes: #180, #270, #275. Compared to before, this PR adds tp_traverse and tp_clear in order to let Python's garbage collector resolve any reference cycles. test_cyclic_gc specifically tests a situation which would fail without opting into the GC.

The addition of __dict__ also means that pickling needs to be handled differently for these classes. The PickleableWithDict test was added for this reason.

If the implementation looks good, I'll add a section to the documentation.

Question about function linkage: This may be a little off-topic, but I noticed it while adding traverse and clear. The existing functions which are passed to Python (generic_type::init, generic_type::new_instance and similar) are all just regular C++ functions. I was under the impression that these needed to be marked with extern "C" in order to get the correct linkage before being passed to Python's C API. Obviously, it works as it is now, but are there any portability concerns since it shouldn't technically be allowed to call a function with C++ linkage from C?

@wjakob
Copy link
Member

wjakob commented Oct 12, 2016

This looks really great -- the GC bits must have been pretty tricky to figure out I guess ;).

Three comments:

  1. Yes, please go ahead and document the new flag.
  2. It's a bummer that we need to pay for the cost of the extra pointer even if dynamic_attr was not specified. However, this should be simple to avoid by extending the "instance" data structure with a fancy version that adds the extra PyObject*. Then based on whether the dynamic_attr bit is set, allocation and deallocation can reserve the right amounts of memory. What do you think?
  3. Good point about the extern "C" linkage. There are probably many other function calls (like the lambda functions) that should technically be marked as well and can't even be marked since the language does not support it. In practice, I suppose that the calling conventions for C and C++ functions are the same for all major systems out there, with the linkage differences just boiling down to symbol name mangling (which is irrelevant when passing around function pointers instead of symbol names). Does that make sense?

Best,
Wenzel

@dean0x7d
Copy link
Member Author

dean0x7d commented Oct 13, 2016

extern "C": Sure, makes sense.

Documentation: I added a new section to the "Object-oriented code" page since it seems to fit there. Let me know if I should move it into "Advanced" instead.

Making PyObject *dict optional: The implementation now looks a bit like a hack, since PyObject *dict is never actually named anywhere, but this is legal C API using tp_dictoffset and _PyObject_GetDictPtr(). I considered some alternatives, but the problem is that offsetof only works for standard layout types, so anything which extends instance by inheritance is immediately disqualified. Having two different POD instance_essentials (with and without a dict) would make offsetof a possibility again, but managing two different bases for instance would require a lot of template code.

@wjakob
Copy link
Member

wjakob commented Oct 14, 2016

Beautiful. No worries about the hack, that seems perfectly reasonable.

@wjakob wjakob merged commit 5c13749 into pybind:master Oct 14, 2016
@dean0x7d dean0x7d deleted the dynamic-attrs branch October 15, 2016 16:54
@dean0x7d
Copy link
Member Author

FYI: Boost.Python has dynamic attributes by default, but it does not opt into the GC. So the situation tested in test_cyclic_gc fails with Boost.Python, which was surprising. The test is explicit, but reference cycles can also appear in subtle ways, e.g. with functions due to variables captured in closures (which was the issue in the previous PR).

Despite some initial trouble, this was actually fun to work on -- I learned some interesting tidbits about CPython.

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