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

fix: <ranges> support for py::tuple and py::list #5314

Merged
merged 8 commits into from
Aug 21, 2024

Conversation

ObeliskGate
Copy link
Contributor

@ObeliskGate ObeliskGate commented Aug 16, 2024

Description

It seems that #4127 has made a mistake there: actually p2325r3 says nothing about C++ sentinels (which always requires semiregular), so it's necessary to add a default constructor for iterator_policies classes.

Moreover, forward_iterator concept requires that:

Value-initialized iterators of the same type may be compared and shall compare equal to other value-initialized iterators of the same type.

and it's tested here:
https://github.com/ObeliskGate/pybind11/blob/74615a04105366647261da83f2980cd5df01b3cd/tests/test_pytypes.cpp#L932-L940
, together with other simple tests.

Suggested changelog entry:

fix and test ``<ranges>`` support for ``py::tuple`` and ``py::list``

@ObeliskGate
Copy link
Contributor Author

There's two tiny questions:

  1. clang had a bug fixed at clang-16 [concepts] deferred substitution into requirements of class template members not implemented llvm/llvm-project#44178, which caused clang with libstdc++ unable to use <ranges>. I've tried to run tests with clang++-15 -fexperimental-library -stdlib=libc++ on my machine, and while the test of <ranges> can pass, another test test_cross_module_exception_translator failed, so I wonder if it's necessary to support it.
  2. It seems that there's no CI that test pypy with C++20, and in this situation it will make generic_iterator<iterator_policies::sequence_slow_readwrite> used by list and tuple in pypy not tested, although I'm sure the behavior is right currently.

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.

include/pybind11/detail/common.h Outdated Show resolved Hide resolved
tests/test_pytypes.cpp Outdated Show resolved Hide resolved
tests/test_pytypes.cpp Outdated Show resolved Hide resolved
tests/test_pytypes.cpp Outdated Show resolved Hide resolved
tests/test_pytypes.cpp Outdated Show resolved Hide resolved
tests/test_pytypes.py Outdated Show resolved Hide resolved
tests/test_pytypes.py Outdated Show resolved Hide resolved
tests/test_pytypes.py Outdated Show resolved Hide resolved
@rwgk
Copy link
Collaborator

rwgk commented Aug 17, 2024

2. It seems that there's no CI that test pypy with C++20

pypy has so many problems and flakes so often, I wouldn't want to add another (flaky) job.

@rwgk
Copy link
Collaborator

rwgk commented Aug 17, 2024

  1. clang had a bug fixed at clang-16 ... so I wonder if it's necessary to support it.

No.

We have so much test coverage across other compilers, we don't need to get that one.

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.

Looks nice, but caveat: I'm not familiar with <ranges>, std::random_access_iterator, std::ranges::views::transform.

I'll merge this after you condense the code around PYBIND11_TEST_PYTYPES_HAS_RANGES.

tests/test_pytypes.cpp Outdated Show resolved Hide resolved
@henryiii henryiii merged commit 2baf9d6 into pybind:master Aug 21, 2024
78 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 21, 2024
@henryiii henryiii changed the title add <ranges> support for py::tuple and py::list fix: <ranges> support for py::tuple and py::list Aug 21, 2024
henryiii added a commit that referenced this pull request Aug 22, 2024
* feat: add `<ranges>` support for `py::tuple` and `py::list`

* fix: format the code

* fix: disable `ranges` in clang < 16

* refactor: move `<ranges>` test macro to `test_pytypes.h`

* refactor: seperate `ranges` test into 3 funcs

* style: compress the if statement

* style: pre-commit fixes

* style: better formatting

---------

Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@ObeliskGate ObeliskGate deleted the ranges branch August 26, 2024 14:27
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 13, 2024
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