Add make_value_iterator #3271
Add make_value_iterator #3271
Conversation
This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common.
include/pybind11/pybind11.h
Outdated
|
|
||
| if (!detail::get_type_info(typeid(state), false)) { | ||
| class_<state>(handle(), "iterator", pybind11::module_local()) | ||
| .def("__iter__", [](state &s) -> state& { return s; }) | ||
| .def("__next__", [](state &s) -> ValueType { | ||
| .def("__next__", [](state &s) -> detail::remove_cv_t<ValueType> { |
There was a problem hiding this comment.
@rwgk I think we need to make sure that this doesnt' cause those edge case issues we were having with removing the const in return types for Eigen
There was a problem hiding this comment.
I could run this through our global testing tonight (after I got a chance to take a closer look at the code).
There was a problem hiding this comment.
@Skylion007 could you elaborate on what those problems were? Is it something that might only show up when iterating over a container of Eigen objects?
There was a problem hiding this comment.
Long story short, Eigen has a special caster that makes it so const Eigen objects are to Numpy arrays with the writable flag off, while non-const Eigen objects have them on so this can actually break some tests even though it's against C++ guidelines / behavior.
There was a problem hiding this comment.
So what would a test for that look like? Something like binding std::vector<const Eigen::Vector3f>, passing it to Python and checking that the writeable flag is off when iterating it? For that matter I'm not even sure it's valid to have a const value type in an STL container.
There was a problem hiding this comment.
To be on the safe side I've removed all the remove_cv_t stuff and put in NOLINTNEXTLINE instead. I also discovered that subtleties in decltype meant in master, make_key_iterator was making copies rather than references. I think that should be fixed in this PR (it's more important for make_value_iterator, since map values are mutable while keys are not), but I still need to add some tests.
There was a problem hiding this comment.
Shall we add a unit test asserting the key is not copied (e.g. delete the copy constructor)?
There was a problem hiding this comment.
Shall we add a unit test asserting the key is not copied (e.g. delete the copy constructor)?
That's exactly the test I've added. I just need to see why it's failing on some compilers in the CI matrix but not others.
|
I'm seeing ~15 breakages (out of a huge number) in the Google-internal global testing (which includes a large number of open source packages). Spot-checking, all failures appear to be similar to the error below. I applied the changes on top of the smart_holder branch (the only way I can test run the global tests), but I don't think that makes a difference with regards to the breakages, except that the pybind11.h line numbers below don't match what's on master. I captured the relevant source code fragments below. The third_party code is from here: Please let me know any suggestions or ideas to try out. I'm traveling today/tomorrow but will try to respond as much as possible. pybind11.h fragments, in order as reference by the error message: third_party/librealsense2/v2_38_1/wrappers/python/pyrs_frame.cpp |
|
Thanks for that test @rwgk. Looking at the code again I see that for some reason I made the |
For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages.
|
@rwgk I've changed the iterator_access classes in a way that I hope will fix things. Could you re-run your battery of tests? |
Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy.
This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen).
Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it.
|
@rwgk I think don't run the Google tests quite yet - there are still some CI failures I need to understand, which seem to be indicating that some compilers (but not all) are trying to make copies where I'm not expecting it. I'll have to investigate next week. |
|
std::iterator and std::pair have ::value_type and ::first_type, ::second_type: I guess the reason to use decltype is that these type names are not always defined in non-std iterator and pairs? |
Iterators aren't required to provide An advantage of |
I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11.
|
It looks like at a minimum ICC is buggy when it comes to Another of the failures is with PGI, but godbolt.org doesn't support PGI so I can't directly probe the issue. I'm going to try some changes to see if I can work around the problem, but it'll be partly trial-and-error since I'll have to test via the CI. |
https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things.
It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible.
|
I've finally gotten the CI to all pass. @rwgk could you try the Google testing again? |
include/pybind11/pybind11.h
Outdated
| #ifndef DOXYGEN_SHOULD_SKIP_THIS // Issue in breathe 4.26.1 | ||
| typename KeyType = decltype((*std::declval<Iterator>()).first), | ||
| typename KeyType = typename detail::iterator_key_access<Iterator>::result_type, | ||
| #endif |
There was a problem hiding this comment.
@Skylion007 I don't know what this breathe issue was, but maybe my change will resolve it? I can try removing the #ifndef but I don't know what symptoms to look for.
There was a problem hiding this comment.
@bmerry From looking at the git blame, the issue is further described here: #2828 . It appears as though not skipping this will hide the output from the reference documentation: #2826 (comment)
There was a problem hiding this comment.
Thanks. If I've followed the chain of causes, I think it's probably no longer an issue (I've run into the same issue with Doxygen myself, where it eats parentheses from decltype expressions), so I've deleted the DOXYGEN_SHOULD_SKIP_THIS guards and we'll see what the CI says.
There was a problem hiding this comment.
@bmerry Also, we could consider bumping Breathe if needed. It's currently pinned to that version via requirements.
|
FYI: I initiated new Google testing. It'll take ~8 hours before the results are in. I verified interactively that the latest version of this PR resolves the realsense breakage reported here before. |
|
Interestingly, this small change increases the binary size by 28kb, I am guess it's the new STL wrappers for the new test cases, just want to make sure our clever new templates and decl aren't adding unnecessary bloat. For a 1% size regression, this diff seems worth it IMO. |
Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues.
I don't have any experience trying to optimise for size, or even know what exactly you're measuring. Can you perhaps try compiling the tests from master against the pybind11.h from this branch? That should tell us what the impact of the restructuring is without the noise from adding more test cases. Also if you're testing this, can you try declaring |
|
I don't have a lot of experience optimizing it either. Our cmake just outputs the size of shared libraries and I wanted to make note of it in case there was an easy fix. |
If you send me the command you run to build and where to look in the output I can try the suggestions I made. |
cmake -S. -B build -DDOWNLOAD_EIGEN=ON -DDOWNLOAD_CATCH=ON -DCMAKE_CXX_STANDARD=17
cmake --build build Should be the one of the last outputs of cmake |
It's not very obvious (it escaped my attention for months): you're looking for the output of this line in tools/libsize.py: |
|
Thanks, I found the file size in the output. On my setup at least, the difference is entirely down to the new tests: if I revert the test code then the size is exactly the same as master. Also adding |
|
The Google testing was successful. I didn't actually review this PR. @rainwoodman offered to help with the review. |
|
FYI @bmerry : ee0c5ee#r56899065 |
|
It's a little hacky, but here's a MWE: diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp
index 66d64726..faa506c6 100644
--- a/tests/test_sequences_and_iterators.cpp
+++ b/tests/test_sequences_and_iterators.cpp
@@ -306,6 +306,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
public:
explicit IntPairs(std::vector<std::pair<int, int>> data) : data_(std::move(data)) {}
const std::pair<int, int>* begin() const { return data_.data(); }
+ const std::pair<int, int>* end() const { return data_.data() + data_.size(); }
private:
std::vector<std::pair<int, int>> data_;
};
@@ -320,6 +321,10 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
.def("nonzero_values", [](const IntPairs& s) {
return py::make_value_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
}, py::keep_alive<0, 1>())
+ // test iterator with keepalive
+ .def("make_iterator_keep_alive", [](IntPairs& self) {
+ return py::make_iterator(self, py::keep_alive<0, 1>());
+ }, py::keep_alive<0, 1>())
;
// test_iterater_referencing
@@ -430,3 +435,4 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
m.def("make_iterator_1", []() { return py::make_iterator<py::return_value_policy::copy>(list); });
m.def("make_iterator_2", []() { return py::make_iterator<py::return_value_policy::automatic>(list); });
}
+
diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py
index 2c73eff2..89e42ce0 100644
--- a/tests/test_sequences_and_iterators.py
+++ b/tests/test_sequences_and_iterators.py
@@ -21,6 +21,10 @@ def test_generalized_iterators():
assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero()) == [(1, 2)]
assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero()) == []
+ with pytest.raises(RuntimeError):
+ m.IntPairs([(1, 2), (3, 4), (0, 5)]).make_iterator_keep_alive()
+
assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero_keys()) == [1, 3]
assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_keys()) == [1]
assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_keys()) == []This doesn't work at runtime in |
|
So in this reproducer, |
|
No, sentinels are something you stop loops with, so need to be deref'd to something comparable to the iterator. This final overload is now unreachable, you can't trigger it if you have any Extras, since everything now matches Sentinel, SFINAE is broken after this PR. We just never tested the final overload with an Extra before. |
|
Thanks for the MWE @henryiii. I should have time for this next week and I can add that MWE to a follow-up PR. In theory it shouldn't be too hard to fix - it's mostly going to be trying to navigate around compiler bug(s), which means the slow painful process of debug-via-CI. |
* Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params
* Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params
* Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params
* Add make_value_iterator (#3271) * Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params * fix: regression in #3271 Co-authored-by: Bruce Merry <1963944+bmerry@users.noreply.github.com>
Description
This is the counterpart to make_key_iterator, and will allow
implementing a
valuemethod inbind_map(although doing so is leftfor a subsequent PR).
I made a few design changes to reduce copy-and-paste boilerplate.
Previously detail::iterator_state had a boolean template parameter to
indicate whether it was being used for make_iterator or
make_key_iterator. I replaced the boolean with a class that determines
how to dereference the iterator. This allows for a generic
implementation of
__next__.I also added the ValueType and Extra... parameters to the iterator_state
template args, because I think it was a bug that they were missing: if
make_iterator is called twice with different values of these, only the
first set has effect (because the state class is only registered once).
There is still a potential issue in that the values of the extra
arguments are latched on the first call, but since most policies are
empty classes this should be even less common.
Suggested changelog entry: