Skip to content

Conversation

dean0x7d
Copy link
Member

This was touched on briefly in PR #445. Similar to that PR, this makes the tuple caster more Pythonic by accepting any sequence.

The only drawback is that this makes overloading on std::tuple and std::vector ambiguous, but shouldn't be a problem in practice. std::vector and std::list are already ambiguous overloads and it doesn't seem to have been an issue so far. 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)).

Regarding the implementation, the tuple caster and function argument loader are split into two different classes. This is needed in order to allow the tuple caster to accept any sequence while keeping the argument loader fast. There is very little overlap between the two classes which makes the separation clean. It’s also good practice not to have completely new functionality in a specialization.

The last commit slightly tweaks make_index_sequence to make it conform to the std version. That way, in C++14 mode, the standard library version can be used which can be a big advantage on newer compilers which make these sequences free, thus removing the possibility of hitting recursive template instantiation limits.

@dean0x7d
Copy link
Member Author

This is likely going to conflict with PR #525 because of the changes to the tuple caster, but I've included zero-element overloads for load/cast to avoid UB as suggested in that PR.

@wjakob
Copy link
Member

wjakob commented Nov 27, 2016

These all look like good changes to me. Factoring out the argument loader is a good idea, it's not particularly related to the tuple caster in any case.

Thoughts from others?

This is needed in order to allow the tuple caster to accept any sequence
while keeping the argument loader fast. There is also very little overlap
between the two classes which makes the separation clean. It’s also good
practice not to have completely new functionality in a specialization.
This is more Pythonic and compliments the std::vector and std::list
casters which also accept sequences.
Newer standard libraries use compiler intrinsics for std::index_sequence
which makes it ‘free’. This prevents hitting instantiation limits for
recursive templates (-ftemplate-depth).
@dean0x7d
Copy link
Member Author

Fixed a small issue: I originally just replaced PyTuple_GET_ITEM with PySequence_ITEM but forgot that one returns a borrowed reference while the other returns a new one. Replaced it with py::sequence now to avoid doing manual reference counting.

@wjakob wjakob merged commit 8c85a85 into pybind:master Dec 3, 2016
@dean0x7d dean0x7d deleted the tuple_caster branch December 3, 2016 23:54
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