Add support for pybind11 Smart_holder branch #269
Conversation
There was a problem hiding this comment.
Thank you for putting this together @kliegeois ! Looks good to me!
-- Could you please add minimal documentation for a new option? I think it would be great if it links to #263 as well as to smart-holder Pybind11 branch.
Thanks,
| #include <sstream> // __str__ | ||
|
|
||
| #include <functional> | ||
| #include <pybind11/smart_holder.h> |
There was a problem hiding this comment.
Right now this test will fail since there is no smart_holder.h header in our Pybind11 fork... 🤔 - any thoughts how we can workaround this? Looking at https://github.com/pybind/pybind11/blob/smart_holder/include/pybind11/smart_holder.h it seems that it does include original pybind11.h but it is not clear why developers have decided to create separate header...
To get started, I see at least the following ways we can work around this issue:
[a] add smart_holder.h symlink pointing to original file to our current Pybind11 fork, - sounds like a error prone solution with some maintenance overhead
[b] somehow mark this particular test as "special" so compilation failure is ignored
[c] allow user to specify which include to use in place of smart_holder.h and instruct test to override that to use standard header
@kliegeois thoughts on this? I am leaning toward [b] unless some there an elegant solution that I missing...
There was a problem hiding this comment.
I would prefer the option [b], I think it is the cleanest option.
There was a problem hiding this comment.
@kliegeois I gave this more thoughts and I think I have better idea: how about if we add one more config directive: pybind11_include_file to allow user to customize it and in the same time remove automatic changing pybind11_include_file value when one or more smart_holder config options was specified? - I think that will give very nice and predictable behavior as well as expand possible code use (for example if for some reason someone need to use different pybind11 header for other reasons). And in the same time this will allow us to fully run the test. Thoughts?
There was a problem hiding this comment.
And if we go with above changes then please also note that pybind11_include_file config option will need short description in documentation. Thanks,
|
Thanks @lyskov for the review! I will add the documentation as requested. |
source/config.hpp
Outdated
| @@ -52,6 +52,7 @@ class Config | |||
| string default_member_rvalue_reference_return_value_policy_ = "pybind11::return_value_policy::automatic"; | |||
| string default_call_guard_ = ""; | |||
| string holder_type_ = "std::shared_ptr"; | |||
| string include_file_ = "pybind11/pybind11.h"; | |||
There was a problem hiding this comment.
Could you please rename this to something like pybind11_include_file_? Otherwise name will be quite confusing (ie include file for what?) since we talk about includes all over Binder code. Thanks,
source/config.hpp
Outdated
| @@ -90,6 +91,7 @@ class Config | |||
| string const &prefix_for_static_member_functions() { return prefix_for_static_member_functions_; } | |||
|
|
|||
| string const &holder_type() const { return holder_type_; } | |||
| string const &include_file() const { return include_file_; } | |||
There was a problem hiding this comment.
same as above: could you please rename this to pybind11_include_file to avoid confusion? Thanks,
| #include <sstream> // __str__ | ||
|
|
||
| #include <functional> | ||
| #include <pybind11/smart_holder.h> |
There was a problem hiding this comment.
@kliegeois I gave this more thoughts and I think I have better idea: how about if we add one more config directive: pybind11_include_file to allow user to customize it and in the same time remove automatic changing pybind11_include_file value when one or more smart_holder config options was specified? - I think that will give very nice and predictable behavior as well as expand possible code use (for example if for some reason someone need to use different pybind11 header for other reasons). And in the same time this will allow us to fully run the test. Thoughts?
| #include <sstream> // __str__ | ||
|
|
||
| #include <functional> | ||
| #include <pybind11/smart_holder.h> |
There was a problem hiding this comment.
And if we go with above changes then please also note that pybind11_include_file config option will need short description in documentation. Thanks,
|
Thanks for the review @lyskov ! |
|
@kliegeois thank you for putting this together! I really like how this is implanted now, - will merge this shortly! |
This PR adds support for the pybind11 smart_holder branch as discussed in #263. @lyskov