Allow binding factory functions as constructors #805

jagerman · 2017-04-17T02:45:49Z

Edit:

Redid this (a few times). The interface is now:

    cls.def(py::init(&factory_function));

where factory_function is some pointer- or holder-generating factory function or lambda of the type that cls binds, such as:

    cls.def(py::init([](int a) { return new MyClass(a); }));

Internally, this still results in a (non-static) __init__ method, but we handle the C++-static-factory <-> python method translation by calling the factory function, then stealing the resulting object internal pointer and holder.

jagerman · 2017-04-17T02:46:57Z

(Credit to @YannickJadoul and @bmerry for the idea).

jagerman · 2017-04-17T03:14:04Z

Also worth noting: this isn't affected by #804, because the def_static("__init__", ...) support here doesn't actually bind a python-side static method, so def("__init__") and def_static("__init__") can be mixed.

wjakob · 2017-04-17T10:48:28Z

This strikes me a fairly intrusive change to support a relatively narrow set of situations. (In particular, the instance_swap() function, and that we need to stick something into the dispatch code regardless of whether this feature is used or not.)

As an alternative, could we have something like:

m.def("__init__", py::factory_adapter(&Example::create));

where the factory_adapter either uses an in-place move constructor, or it takes a py::object as self argument and then does something similar to instance_swap.

jagerman · 2017-04-17T13:48:12Z

As an alternative, could we have something like:
m.def("__init__", py::factory_adapter(&Example::create));
where the factory_adapter either uses an in-place move constructor, or it takes a py::object as self argument and then does something similar to instance_swap.

Yeah, that should be doable. I'll take another shot at this in the next day or two.

jagerman · 2017-04-17T13:50:21Z

And that MSVC job failure is more bizarre than ever.

YannickJadoul · 2017-04-17T14:47:09Z

it takes a py::object as self argument and then does something similar to instance_swap.

I don't want to come across whiny, but could I bring up that gist I've mentioned in the gitter chat, again?
https://gist.github.com/YannickJadoul/38dc4ef8f0faa56fa10824626886edab

I'm sure it could be improved a lot with better knowledge of pybind11, but what it more or less does is:

Define a class unconstructed_holder which takes care of the deallocation of the allocated memory for the instance, then constructs the holder immediately.
Define type_caster<unconstructed_holder> that is only able to load a py::instance, not just extracting the pointer to the instance but also the memory reserved for the holder

jagerman · 2017-04-19T00:28:52Z

Redesigned this.

The redesign is far less intrusive, and avoids some unnecessary Python object creation in several cases.

I still want to add some tests to verify all the various edge cases (e.g. for things like returning an alias pointer or value, or a subclass pointer).

wjakob · 2017-04-19T00:30:19Z

docs/advanced/classes.rst

+        // Bind an existing pointer-returning factory function:
+        .def(py::init_factory(&Example::create))
+        // Similar, but returns the pointer wrapped in a holder:
+        .def("__init__", []() {


init_factory missing.

wjakob · 2017-04-19T00:37:46Z

Nifty -- +1 for covering all the different kinds of things that could be returned. Since the detail::init_factory code got somewhat large, I'm wondering if it might make sense to move it into a separate header file (e.g. pybind11/factory.h) since it addresses a fairly specific requirement.

jagerman · 2017-04-19T00:42:32Z

As far as organization, moving it to another header is a good idea. It can't quite be all moved, though: the class_::init(py::init_factory &&) member function is still needed (because it needs to pass the class_<...> type to execute()).

wjakob · 2017-04-19T01:14:19Z

Yep, that is to be expected. With a forward declaration, it should be possible though, no?.

jagerman · 2017-04-19T02:01:14Z

I assume you meant making it an optional (i.e. requires an extra include before using) header.

YannickJadoul · 2017-04-19T15:27:14Z

I don't know about all the subtleties of handling e.g. py::handle or py::object, but this seems a really nice solution to the problem to me, especially the way it hardly requires an internal change :-)
Trying this out on my code and test case I'd written works like a charm.

I've got two possibly useful things:

As far as I know, init_holder is still called afterwards, which might mess things up by overwriting the holder once again? I.e., since the owned field of the instance is still true, init_holder will think it needs to wrap the pointer again in a holder? (But maybe I've overlooked something.)
Just a detail, but nevertheless: I believe your alias_constructible does not need SFINAE but can do with just a using alias_constructible = std::integral_constant<bool, that_long_boolean_expression> ?

But for the rest, just "wow", again!

jagerman · 2017-04-19T15:40:20Z

Re: init_holder: I added a check to the if-constructor-then-init-holder call in dispatch, but apparently it got lost when I was cleaning up the commit: good catch!

The SFINAE was necessary because the bool_constant version fails when the type has no alias because Alias<Class> is void and so Alias<Class> && is taking a reference to void, which is invalid.

YannickJadoul · 2017-04-19T15:48:31Z

The SFINAE was necessary because the bool_constant version fails when the type has no alias because Alias is void and so Alias && is taking a reference to void, which is invalid.

Aha, my bad, sorry

But thanks again for the quick and elegant solution!

jagerman · 2017-04-20T20:14:22Z

I've added various tests and fixes related to value and alias initialization, and moved the tests out into a new test_factory_constructors rather than bulking up method_and_attributes with things that are only sort-of methods.

dean0x7d

As I've said before, I support the inclusion of the factory by default and the changes to the docs to prefer py::init(...) over placement new. The lazy allocation added to the latest revision is also great and removes any runtime overhead concerns of the factory. It also gives users more freedom regarding various new operators (avoiding placement new), which is a nice bonus.

However, the latest commit, which implements py::init<...>() in terms of the factory, results in a pretty large +5% increase in binary size and about +2% for compile time (in both cases, not counting the new tests).

This could be addressed either by reverting the py::init<...>() changes or by further slimming down the factory implementation. Preferably the latter. A comment below points out the possibility of replacing a couple of templates with a single non-template function. For me, that reduced the binary size gap to +2% (from +5%). More code could similarly be detemplatized to bring the binary size back on par with the original py::init<...>().

dean0x7d · 2017-08-07T16:02:36Z

include/pybind11/factory_support.h

+        #if defined(PYBIND11_CPP14)
+        cl.def("__init__", [cl_type, func = std::move(class_factory)]
+        #else
+        CFuncType func(std::move(class_factory));


const auto &func = class_factory; should be enough for the capture (just copy, instead of move + copy). And similar below for class_func and alias_func.

dean0x7d · 2017-08-07T17:44:11Z

include/pybind11/factory_support.h

+NAMESPACE_END(factory)
+
+// Simple, load-only type caster that does a type check and value_and_holder construction.
+template <typename T> class type_caster<factory::v_h_proxy<T>> {


As far as I can see, these are two extra template instantiations per class (proxy + caster specialization) which could be replaced with a non-template function like the one below (in that case the type of the first argument of the __init__ lambda changes from v_h_proxy<Cpp<Class>> & to object).

PYBIND11_NOINLINE inline value_and_holder load_v_h(handle src, type_info *tinfo) { if (!src || !tinfo) throw type_error(); auto *inst = reinterpret_cast<instance *>(src.ptr()); auto result = inst->get_value_and_holder(tinfo, false); if (!result.inst) throw type_error(); return result; }

dean0x7d · 2017-08-07T18:12:41Z

include/pybind11/factory_support.h

+        no_nullptr(ptr);
+        if (Py_TYPE(v_h.inst) != cl_type) {
+            // Inherited from on the Python side: an alias instance is required
+            if (!is_alias<Class>(ptr)) {


Is this code path covered by the tests? Commenting out everything below except for throw type_error(...) doesn't seem to produce any test errors.

It should have been, but may have been dropped accidentally somewhere along the line. I'll investigate.

dean0x7d · 2017-08-07T18:33:47Z

include/pybind11/factory_support.h

+    static void construct(value_and_holder &v_h, Cpp<Class> &&result, PyTypeObject *) {
+        static_assert(std::is_move_constructible<Cpp<Class>>::value,
+            "pybind11::init() return-by-value factory function requires a movable class");
+        new (preallocate(v_h)) Cpp<Class>(std::move(result));


Any reason not to do deallocate(v_h) = new Cpp<Class>(std::move(result));? Here, and in other places where preallocate. It would reduce the reliance on placement new.

preallocate doesn't do anything if the value is already preallocated, so if you mix old-style placement-new __init__s with factory inits you get fewer allocation/deallocations. If placement-new constructors are rare/deprecated, it seems a reasonable approach though.

(And on a side note, non-factory inits have a pretty big issue with the constructor being invoked multiple time if being used under multiple inheritance, so discouraging them is probably a good thing).

dean0x7d · 2017-08-07T19:22:39Z

include/pybind11/pybind11.h

@@ -203,6 +199,8 @@ class cpp_function : public function {
                a.descr = strdup(a.value.attr("__repr__")().cast<std::string>().c_str());
        }

+        rec->is_constructor = !strcmp(rec->name, "__init__") || !strcmp(rec->name, "__setstate__");


Why did this need to be moved?

It was part of an earlier commit that was since dropped to fix the constructor signature when the first argument was a handle, but that got replaced with v_h_proxy<Cpp<Class>>. I think you want me to go back to that, though.

The constructor signature rewriting got reintroduced with removal of the proxy class, so I kept this moved.

dean0x7d · 2017-08-07T19:24:05Z

include/pybind11/pybind11.h

    }

+    template <typename, typename, typename, typename, typename...> friend struct detail::factory::init;


Doesn't need to be friend.

dean0x7d · 2017-08-07T19:51:45Z

include/pybind11/factory_support.h

+
+// Makes sure the `value` for the given value_and_holder is not allocated; if it is, deallocate it.
+// Returns the (null) value pointer reference.
+inline void *&deallocate(value_and_holder &v_h) {


Under what circumstances is the value already allocated? If __init__ is called multiple times or something else?

BTW In this PR, a good chunk of the doc comments for these internal functions are just restating what the code is doing, but not really why that needs to be done, which is the more important part. E.g. in this case, I find that if (v_h) v_h.type->dealloc(v_h); is a more concise statement of what is happening compared to the comment, but neither really say why that's needed.

One possible case is when you have overloaded constructors when you have mixed placement new and init factory constructors, such as:

cl.def("__init__", [](Class &self, double i) { new (&self) Class(i); }); cl.def("__init__", py::init([](int j) { return new Class(j); });

when called from Python as Class(3) to go to the second overload. Because the lazy allocation happens during argument loading, the &self will get preallocated even though the overload fails, and the other init factories need to deal with it.

I suppose we could deprecated placement new constructors and get rid of this eventually (i.e. in an eventual 3.0), in which case we wouldn't need to worry about this case.

Re: comments, that's a fair criticism; I'll edit them to be more "why" than "what".

Ah, that makes sense with the mixed constructors. Deprecating the placement new constructors sounds good to me.

jagerman · 2017-08-13T03:05:54Z

I've been a bit swamped with other things for the last few days, but I'll try to get this PR updated tomorrow.

jagerman · 2017-08-13T21:49:08Z

The last commit just pushed should address the outstanding comments.

There is still some growth in .so size (~19KB) from the py::init-via-factory implementation. This is buying us a fairly important fix for Python-side multiple inheritance (where multiple __init__s will invoke multiple C++ constructions on the same object) that can't be easily avoided with the previous placement-new implementation, so I think it's worthwhile. I tried to extract some other bits into common code, but couldn't get anything that made a noticeable difference in .so size.

If all looks good, there's one last cleanup I think we should do which is to rename factory_support.h to init_support.h and also move the struct init and struct init_alias into it from pybind11.h. Neither is very large, but they seem logically well matched with the factory code (and takes a bit of detail code out of the main header).

jagerman · 2017-08-14T03:46:48Z

I've renamed factory_support.h to detail/init.h to match #1001. I also moved the various static functions in the factory class into free templated functions in the namespace, then used them to implement a more direct path for the common standard py::init<...>() constructors, which further cuts down the .so size increase (and should also make regular constructors slightly faster--they save the extra function pointer dereference and call that using factory functions would incur).

jagerman · 2017-08-14T03:50:32Z

(The change looks bigger than it actually is; most of the commit diff is just from moving the static functions in the factory class out, and moving the detail::init/detail::init_alias structs into detail/init.h).

dean0x7d

This looks good to me overall. Moving all the helper functions out of the factory class is very nice. I left some comments below, but nothing major. The file size is significantly lower than the previous iteration of this PR. I think the current minor increase is well worth it for the multiple inheritance fix and it also reduces the reliance on placement-new and eliminates the allocation/construction split in most cases, which is a nice advantage.

Oh, there seems to be a bit of a leak in the test output capture: https://travis-ci.org/pybind/pybind11/jobs/264412442#L1305

(I've been experimenting a bit and I think there's a way to shave off a bit more size from the binary, but I need to test it more thoroughly and I don't want to hold up this PR anymore. I think it's good as is and I'll see about this extra size saving separately.)

dean0x7d · 2017-08-16T12:44:32Z

include/pybind11/pybind11.h

@@ -244,6 +242,12 @@ class cpp_function : public function {
                                     .cast<std::string>() + ".";
 #endif
                    signature += tinfo->type->tp_name;
+                } else if (rec->is_constructor && detail::same_type(typeid(handle), *t) && rec->scope) {


Does this match only self or any argument of type handle? Add arg_index == 0?

dean0x7d · 2017-08-16T13:05:55Z

include/pybind11/detail/init.h

+
+// Takes a Cpp pointer and returns true if it actually is a polymorphic Alias instance.
+template <typename Class, enable_if_t<Class::has_alias, int> = 0>
+static bool is_alias(Cpp<Class> *ptr) {


Leftover from old implementation: this and many of the functions below are static but they shouldn't be.

dean0x7d · 2017-08-16T13:08:05Z

include/pybind11/detail/init.h

+    return dynamic_cast<Alias<Class> *>(ptr) != nullptr;
+}
+// Failing fallback version of the above for a no-alias class (always returns false)
+template <typename Class>


Unused template parameters should be unnamed or commented out.

dean0x7d · 2017-08-16T15:31:27Z

tests/test_factory_constructors.py

+
+@pytest.unsupported_on_py2
+@pytest.mark.parametrize('which, bad', [(1, 1), (1, 2), (6, 1), (6, 2), (6, 3), (6, 4)])
+def test_invalid_self(which, bad, capture):


Unused capture.

That was to suppress the capture leak (which it does on my system, but apparently doesn't on one of the travis-ci builds).

I don't quite understand why that capture leak is happening at all, though. Could the mark.parametrized be involved somehow?

Deparameterizing it worked around the issue.

dean0x7d · 2017-08-16T15:31:50Z

tests/test_factory_constructors.py

+                a = m.TestFactory2(tag.pointer, 1)
+                m.TestFactory1.__init__(a, tag.pointer)
+            elif bad == 2:
+                m.TestFactory1.__init__(NotPybindDerived.__new__(NotPybindDerived), tag.pointer)


Why __new__?

I was looking for something allocated but uninitialized. Will it hurt anything? (Edit: It also came straight out of one of the suggestions by @YannickJadoul earlier in the PR).

Updated to just pass an ordinary non-pybind instance (essentially matching to previous case, except for the non-pybind aspect).

It doesn't hurt anything I just thought it looked like unusual syntax for the functionally equivalent NotPybindDerived(). In general, for a native Python class A, the constructor A() = A.__new__() + A.__init__(). If A doesn't have __init__() then A() == A.__new__(), which is the case for NotPybindDerived in the test.

dean0x7d · 2017-08-16T16:15:15Z

include/pybind11/detail/init.h

+};
+
+// Implementation class for py::init(Func) and py::init(Func, AliasFunc)
+template <typename CFunc, typename CReturn, typename AFuncIn, typename AReturn, typename... Args> struct factory {


CReturn and AReturn are not used anymore.

dean0x7d · 2017-08-17T00:23:00Z

include/pybind11/detail/init.h

+// Fallback of the above for aliases without an `Alias(Cpp &&) constructor; does nothing and
+// returns false to signal the non-availability of the special constructor.
+template <typename Class, enable_if_t<!std::is_constructible<Alias<Class>, Cpp<Class> &&>::value, int> = 0>
+static constexpr bool construct_alias_from_cpp(value_and_holder &, Cpp<Class> &&) { return false; }


The control flow built around this feels a bit more complicated than it should be. This return false results in two different throw type_error messages below, but they have the same cause. Consider switching to void functions and place the throw statement in the false fallback. (I'm also using simple tag dispatch in the example below because it's a nice lightweight option, especially for binary overloads, and it also nicely maps to the future if constexpr.)

template <typename Class> using is_alias_constructible = std::is_constructible<Alias<Class>, Cpp<Class> &&>; template <typename Class> void construct_alias_from_cpp(std::true_type /*is_alias_constructible*/, value_and_holder &v_h, Cpp<Class> &&base) { deallocate(v_h) = new Alias<Class>(std::move(base)); } template <typename Class> [[noreturn]] void construct_alias_from_cpp(std::false_type /*is_alias_constructible*/, value_and_holder &, Cpp<Class> &&) { throw type_error("pybind11::init(): unable to convert returned instance to required " "alias class: no `Alias<Class>(Class &&)` constructor available"); }

if (Class::has_alias && need_alias && !is_alias<Class>(ptr)) { ... construct_alias_from_cpp<Class>(is_alias_constructible<Class>{}, v_h, std::move(*ptr)); } else { deallocate(v_h) = ptr; }

We currently allocate instance values when creating the instance itself (except when constructing the instance for a `cast()`), but there is no particular reason to do so: the instance itself and the internals (for a non-simple layout) are allocated via Python, with no reason to expect better locality from the invoked `operator new`. Moreover, it makes implementation of factory function constructors trickier and slightly less efficient: they don't use the pre-eallocate the memory, which means there is a pointless allocation and free. This commit makes the allocation lazy: instead of preallocating when creating the instance, the allocation happens when the instance is first loaded (if null at that time). In addition to making it more efficient to deal with cases that don't need preallocation, this also allows for a very slight performance increase by not needing to look up the instances types during allocation. (There is a lookup during the eventual load, of course, but that is happening already).

`function_signature_t` extracts the function type from a function, function pointer, or lambda. `is_lambda` (which is really `is_not_a_function_or_pointer_or_member_pointer`, but that name is a bit too long) checks whether the type is (in the approprate context) a lambda. `is_function_pointer` checks whether the type is a pointer to a function.

An alias can be used for two main purposes: to override virtual methods, and to add some extra data to a class needed for the pybind-wrapper. Both of these absolutely require that the wrapped class be polymorphic so that virtual dispatch and destruction, respectively, works.

This allows you to use: cls.def(py::init(&factory_function)); where `factory_function` returns a pointer, holder, or value of the class type (or a derived type). Various compile-time checks (static_asserts) are performed to ensure the function is valid, and various run-time type checks where necessary. Some other details of this feature: - The `py::init` name doesn't conflict with the templated no-argument `py::init<...>()`, but keeps the naming consistent: the existing templated, no-argument one wraps constructors, the no-template, function-argument one wraps factory functions. - If returning a CppClass (whether by value or pointer) when an CppAlias is required (i.e. python-side inheritance and a declared alias), a dynamic_cast to the alias is attempted (for the pointer version); if it fails, or if returned by value, an Alias(Class &&) constructor is invoked. If this constructor doesn't exist, a runtime error occurs. - for holder returns when an alias is required, we try a dynamic_cast of the wrapped pointer to the alias to see if it is already an alias instance; if it isn't, we raise an error. - `py::init(class_factory, alias_factory)` is also available that takes two factories: the first is called when an alias is not needed, the second when it is. - Reimplement factory instance clearing. The previous implementation failed under python-side multiple inheritance: *each* inherited type's factory init would clear the instance instead of only setting its own type value. The new implementation here clears just the relevant value pointer. - dealloc is updated to explicitly set the leftover value pointer to nullptr and the `holder_constructed` flag to false so that it can be used to clear preallocated value without needing to rebuild the instance internals data. - Added various tests to test out new allocation/deallocation code. - With preallocation now done lazily, init factory holders can completely avoid the extra overhead of needing an extra allocation/deallocation. - Updated documentation to make factory constructors the default advanced constructor style. - If an `__init__` is called a second time, we have two choices: we can throw away the first instance, replacing it with the second; or we can ignore the second call. The latter is slightly easier, so do that.

This reimplements the py::init<...> implementations using the various functions added to support `py::init(...)`, and moves the implementing structs into `detail/init.h` from `pybind11.h`. It doesn't simply use a factory directly, as this is a very common case and implementation without an extra lambda call is a small but useful optimization. This, combined with the previous lazy initialization, also avoids needing placement new for `py::init<...>()` construction: such construction now occurs via an ordinary `new Type(...)`. A consequence of this is that it also fixes a potential bug when using multiple inheritance from Python: it was very easy to write classes that double-initialize an existing instance which had the potential to leak for non-pod classes. With the new implementation, an attempt to call `__init__` on an already-initialized object is now ignored. (This was already done in the previous commit for factory constructors). This change exposed a few warnings (fixed here) from deleting a pointer to a base class with virtual functions but without a virtual destructor. These look like legitimate warnings that we shouldn't suppress; this adds virtual destructors to the appropriate classes.

jagerman · 2017-08-17T05:58:19Z

Fixed up everything and squashed the commits down; I think this is ready to go (once the builds finish).

jagerman · 2017-08-17T13:33:38Z

Merged!

wjakob · 2017-08-21T20:45:54Z

include/pybind11/detail/init.h

+        auto *cl_type = get_type_info(typeid(Cpp<Class>));
+        cl.def("__init__", [cl_type](handle self_, Args... args) {
+            auto v_h = load_v_h(self_, cl_type);
+            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)


I stumbled across this comment (duplicate init calls) while looking at init.h and was a bit puzzled. I wasn't able to find what the "see above" comment refers to. Would you mind clarifying this?

^ @jagerman

Ah, the code got shuffled around; the comment it's referring to is at lines 258/259 now. I'll fix it up on master.

(They used to be right next to each other).

Fixed in 9f6a636.

Ah, just got a conflict with that commit :) Those checks are gone now in #1014.

That was pretty incredibly bad timing :)

It was only necessary because of a bug the pull request (pybind/pybind11#805).

jagerman force-pushed the factory-constructors branch from cf0dcce to 462db1b Compare April 17, 2017 03:04

jagerman force-pushed the factory-constructors branch 3 times, most recently from 48665d6 to 2eb05d6 Compare April 17, 2017 06:12

jagerman changed the title ~~Allow binding factory functions as constructors~~ WIP: Allow binding factory functions as constructors Apr 17, 2017

jagerman force-pushed the factory-constructors branch from 2eb05d6 to 16567c3 Compare April 19, 2017 00:23

jagerman changed the title ~~WIP: Allow binding factory functions as constructors~~ Allow binding factory functions as constructors, v2 Apr 19, 2017

jagerman changed the title ~~Allow binding factory functions as constructors, v2~~ WIP: Allow binding factory functions as constructors, v2 Apr 19, 2017

wjakob reviewed Apr 19, 2017

View reviewed changes

jagerman force-pushed the factory-constructors branch 2 times, most recently from fcef625 to dfaec31 Compare April 19, 2017 03:53

jagerman force-pushed the factory-constructors branch 2 times, most recently from 513bfaf to 650b65b Compare April 20, 2017 20:12

jagerman force-pushed the factory-constructors branch 2 times, most recently from d8d66ea to 19d0654 Compare April 20, 2017 20:49

jagerman force-pushed the factory-constructors branch 4 times, most recently from 3ba8721 to b52a0dc Compare August 3, 2017 14:37

dean0x7d requested changes Aug 7, 2017

View reviewed changes

dean0x7d added this to the v2.2 milestone Aug 13, 2017

jagerman force-pushed the factory-constructors branch from b52a0dc to 972a4de Compare August 13, 2017 20:45

jagerman force-pushed the factory-constructors branch from a3c7608 to 64075f9 Compare August 13, 2017 22:04

dean0x7d mentioned this pull request Aug 13, 2017

Move internal headers into detail subdirectory #1001

Merged

jagerman force-pushed the factory-constructors branch from d6aa3ff to 2629ad0 Compare August 14, 2017 15:49

dean0x7d approved these changes Aug 17, 2017

View reviewed changes

jagerman added 3 commits August 16, 2017 22:23

jagerman force-pushed the factory-constructors branch 2 times, most recently from 8fd16e2 to 2dc9078 Compare August 17, 2017 04:36

jagerman added 2 commits August 17, 2017 01:54

jagerman force-pushed the factory-constructors branch from 2dc9078 to 8a48285 Compare August 17, 2017 05:56

jagerman merged commit c4e1800 into pybind:master Aug 17, 2017

wjakob reviewed Aug 21, 2017

View reviewed changes

dean0x7d mentioned this pull request Aug 21, 2017

Reduce binary size overhead of new-style constructors #1014

Merged

bmerry added a commit to ska-sa/katsdpimager that referenced this pull request Sep 12, 2019

Remove the lambda wrapping of make_visibility_collector

03bebdc

It was only necessary because of a bug the pull request (pybind/pybind11#805).

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#805

Closed

		}

		template <typename, typename, typename, typename, typename...> friend struct detail::factory::init;

Allow binding factory functions as constructors #805

Allow binding factory functions as constructors #805

Conversation

jagerman commented Apr 17, 2017 • edited Loading

jagerman commented Apr 17, 2017

jagerman commented Apr 17, 2017

wjakob commented Apr 17, 2017

jagerman commented Apr 17, 2017

jagerman commented Apr 17, 2017

YannickJadoul commented Apr 17, 2017

jagerman commented Apr 19, 2017

Choose a reason for hiding this comment

wjakob commented Apr 19, 2017 • edited Loading

jagerman commented Apr 19, 2017

wjakob commented Apr 19, 2017

jagerman commented Apr 19, 2017

YannickJadoul commented Apr 19, 2017

jagerman commented Apr 19, 2017

YannickJadoul commented Apr 19, 2017

jagerman commented Apr 20, 2017

dean0x7d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Aug 13, 2017

jagerman commented Aug 13, 2017

jagerman commented Aug 14, 2017 • edited Loading

jagerman commented Aug 14, 2017

dean0x7d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Aug 17, 2017

jagerman commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dean0x7d Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Apr 17, 2017 •

edited

Loading

wjakob commented Apr 19, 2017 •

edited

Loading

jagerman commented Aug 14, 2017 •

edited

Loading

jagerman Aug 17, 2017 •

edited

Loading

dean0x7d Aug 21, 2017 •

edited

Loading