Make PYBIND11_OBJECT_CVT only convert if the type check fails #979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jagerman
force-pushed
the
object-convert-check
branch
from
39d1b19
to
42e3eba
Compare
dean0x7d
reviewed
Aug 4, 2017
There was a problem hiding this comment.
Yeah, obj.cast<py::list>() making a copy was an oversight on my part. The fix applied in this PR results in a much more natural syntax.
I'm a bit sad about py::list(obj) not being a guaranteed copy (inconsistency with Python), but as you mentioned, it's not really viable and would create an inconsistency within C++ which is worse than having a C++ <-> Python inconsistency.
include/pybind11/embed.h
Outdated
| @@ -99,7 +99,7 @@ inline void initialize_interpreter(bool init_signal_handlers = true) { | |||
| Py_InitializeEx(init_signal_handlers ? 1 : 0); | |||
|
|
|||
| // Make .py files in the working directory available by default | |||
| auto sys_path = reinterpret_borrow<list>(module::import("sys").attr("path")); | |||
| list sys_path(module::import("sys").attr("path")); | |||
| sys_path.append("."); | |||
There was a problem hiding this comment.
No need for an intermediate variable anymore: module::import("sys").attr("path").cast<py::list>().append("."); Makes it more obvious that the path is being modified in place.
include/pybind11/pytypes.h
Outdated
| @@ -731,7 +731,12 @@ NAMESPACE_END(detail) | |||
| #define PYBIND11_OBJECT_CVT(Name, Parent, CheckFun, ConvertFun) \ | |||
| PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ | |||
| /* This is deliberately not 'explicit' to allow implicit conversion from object: */ \ | |||
| Name(const object &o) : Parent(ConvertFun(o.ptr()), stolen_t{}) { if (!m_ptr) throw error_already_set(); } | |||
| Name(const object &o) \ | |||
| : Parent(o && CheckFun(o.ptr()) ? o.inc_ref() : handle(ConvertFun(o.ptr())), stolen_t{}) \ | |||
There was a problem hiding this comment.
Best not to duplicate the check_() function (and similarly below):
Parent(check_(o) ? o.inc_ref().ptr() : ConvertFun(o.ptr()), stolen_t{})Currently types that are capable of conversion always call their convert
function when invoked with a `py::object` which is actually the correct
type. This means that code such as `py::cast<py::list>(obj)` and
`py::list l(obj.attr("list"))` make copies, which was an oversight
rather than an intentional feature.
While at first glance there might be something behind having
`py::list(obj)` make a copy (as it would in Python), this would be
inconsistent when you dig a little deeper because `py::list(l)`
*doesn't* make a copy for an existing `py::list l`, and having an
inconsistency within C++ would be worse than a C++ <-> Python
inconsistency.
It is possible to get around the copying using a
`reinterpret_borrow<list>(o)` (and this commit fixes one place, in
`embed.h`, that does so), but that seems a misuse of
`reinterpret_borrow`, which is really supposed to be just for dealing
with raw python-returned values, not `py::object`-derived wrappers which
are supposed to be higher level.
This changes the constructor of such converting types (i.e. anything
using PYBIND11_OBJECT_CVT -- `str`, `bool_`, `int_`, `float_`, `tuple`,
`dict`, `list`, `set`, `memoryview`) to reference rather than copy when
the check function passes.
It also adds an `object &&` constructor that is slightly more efficient
by avoiding an inc_ref when the check function passes.
jagerman
force-pushed
the
object-convert-check
branch
from
42e3eba
to
19a4860
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently types that are capable of conversion always call their convert
function when invoked with a
py::objectwhich is actually the correcttype. This means that code such as
py::cast<py::list>(obj)andpy::list l(obj.attr("list"))make copies, which seems an oversightrather than an intentional feature.
While at first glance there might be something behind having
py::list(obj)make a copy (as it would in Python), this would beinconsistent when you dig a little deeper because
py::list(l)doesn't make a copy for an existing
py::list l.It is possible to work around the issue using a
reinterpret_borrow<list>(o)(and this commit fixes one place, inembed.h, that does so), but that seems a misuse ofreinterpret_borrow, which is really supposed to be just for dealingwith raw python-returned values, not
py::object-derived wrapperswhich are supposed to be higher level.
This changes the constructor of such converting types (i.e. anything
using PYBIND11_OBJECT_CVT --
str,bool_,int_,float_,tuple,dict,list,set,memoryview) to reference rather than copy whenthe check function passes.
(Cc and thanks to @apollo13, who brought my attention to this issue).