Skip to content

Conversation

pschella
Copy link
Contributor

In the current implementation of the STL container casters functions which take std::vector or std::list arguments in C++ will only accept list in Python. This is not very Pythonic. Users will expect the functions to be callable with any other sequence type (e.g. tuple).

This PR implements that capability. It only affects std::vector and std::list to not complicate overloading on std::tuple or std::pair arguments, but could potentially be extended to cover those as well.

Note that this only affects load, functions returning std::vector or std::list will still return a Python list.

@dean0x7d
Copy link
Member

dean0x7d commented Oct 15, 2016

Nice. Python usually doesn't care about the specific sequence type so this makes a lot of sense. (This would also allow conversions of 1D ndarray to std::vector.)

IMHO It would be nice to have this for std::tuple as well. Although, there are 2 issues in that case, as far as I can see. I don't think either one is a showstopper, but some discussion/work is needed -- probably best to leave it for a separate PR. The two things are:

  1. As you mentioned, it would make overloading on std::tuple and std::vector ambiguous. But unless I'm missing something, I don't think this would be a problem in practice. First off, std::vector and std::list are already ambiguous overloads and it doesn't seem to have been an issue so far. Similarly, I don't think there's a lot of sense in exporting a function overloaded on std::tuple and std::vector to Python -- it's not very Pythonic to expect different results from foo([1, 2, 3]) and foo((1, 2, 3)).
  2. The tuple caster also serves as the argument loader for function calls. This is pretty critical so it wouldn't be good to slow it down with the more generic sequence protocol. However, it may be nice to actually separate the argument loader from the tuple caster. That would make it possible to make the argument loader more specialized and faster (e.g. tuple and size checks are redundant since dispatcher already ensures this) and on the other hand make the tuple caster more versatile (accept sequences). I think this would be a win for both.

@wjakob
Copy link
Member

wjakob commented Oct 15, 2016

This looks good to me. Merged.

@dean0x7d: With regards to the tuple caster and function calls: this already uses the special load_args specialization that uses PyTuple_GET_ITEM and avoids all of the size checks etc. If tuples loading is to be extended to sequences, it would just be important to preserve the fast path for function arguments.

@wjakob wjakob merged commit 946f897 into pybind:master Oct 15, 2016
@dean0x7d
Copy link
Member

Ah, I didn't see the fast path for load_args. Cool.

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.

3 participants