Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: perfectly forward all make_iterator args #3980

Merged

Conversation

Skylion007
Copy link
Collaborator

Description

This forwards all args in make_iterator. This should be more efficient and allow downstream functions to use the rvalue / lvalue nature of the args).

  • I also added a missing move argument and emplace_back for an lambda.

Suggested changelog entry:

* Perfect forward all args of make_iterator.

@Skylion007 Skylion007 requested review from rwgk and henryiii May 28, 2022 17:19
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain my rationale, in contrast to PR #3970: here you are just inserting a few && and std::move(), and use emplace_back() instead of push_back(). If we lose any of those, optimizations again, so be it.

In PR #3970, you are adding new functions and member functions. That's clearly a level up in terms of code complexity, and there is also more danger that overloads get lost unnoticed. I want to maintain a quality standard that includes: if an entire (member) function is removed accidentally, we need to see test breakages.

@Skylion007 Skylion007 merged commit 8da58da into pybind:master May 28, 2022
@Skylion007 Skylion007 deleted the skylion007/make-iterator-pforwarding branch May 28, 2022 23:58
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 28, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
@henryiii
Copy link
Collaborator

FYI, I think this change is causing ambiguous resolution:

In file included from /Users/henryschreiner/git/scikit-hep/boost-histogram/src/register_axis.cpp:10:
/Users/henryschreiner/git/scikit-hep/boost-histogram/include/bh_python/register_axis.hpp:252:24: error: call to 'make_iterator' is ambiguous
                return py::make_iterator(begin, end);
                       ^~~~~~~~~~~~~~~~~
/Users/henryschreiner/git/scikit-hep/boost-histogram/src/register_axis.cpp:22:19: note: in instantiation of function template specialization 'register_axis<boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<0>>>' requested here
        auto ax = register_axis<T>(mod);
                  ^
/Users/henryschreiner/git/scikit-hep/boost-histogram/src/register_axis.cpp:28:5: note: in instantiation of function template specialization 'register_axis_each<boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<0>>, boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<1>>, boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<2>>, boost::histogram::axis::regular<double, boost::use_default, metadata_t>, boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<11>>, boost::histogram::axis::regular<double, boost::use_default, metadata_t, boost::histogram::axis::option::bitset<6>>, axis::regular_numpy, (lambda at /Users/henryschreiner/git/scikit-hep/boost-histogram/src/register_axis.cpp:34:50)>' requested here
    register_axis_each<axis::regular_none,
    ^
/Users/henryschreiner/git/scikit-hep/boost-histogram/extern/pybind11/include/pybind11/pybind11.h:2373:10: note: candidate function [with Policy = pybind11::return_value_policy::reference_internal, Iterator = iterator &, Sentinel = iterator &, ValueType = pybind11::tuple, Extra = <>]
iterator make_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra) {
         ^
/Users/henryschreiner/git/scikit-hep/boost-histogram/extern/pybind11/include/pybind11/pybind11.h:2425:10: note: candidate function [with Policy = pybind11::return_value_policy::reference_internal, Type = iterator, Extra = <iterator &>]
iterator make_iterator(Type &value, Extra &&...extra) {
         ^

I can fix it by adding a std::move, but I'd think that shouldn't be required to avoid ambiguity.

Skylion007 added a commit to Skylion007/pybind11 that referenced this pull request Oct 11, 2022
henryiii pushed a commit that referenced this pull request Oct 21, 2022
* Revert "chore: perfectly forward all make_iterator args (#3980)"

This reverts commit 8da58da.

* Redo unrelated optimization in commit

* Add tests for ambiguous overloads
henryiii pushed a commit to henryiii/pybind11 that referenced this pull request Oct 21, 2022
* Revert "chore: perfectly forward all make_iterator args (pybind#3980)"

This reverts commit 8da58da.

* Redo unrelated optimization in commit

* Add tests for ambiguous overloads
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