feat: reapply fixed version of #3271 (#3293)

* 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>

tests/test_sequences_and_iterators.py

@@ -36,6 +36,10 @@ def test_generalized_iterators():
     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()) == []
 
+    assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero_values()) == [2, 4]
+    assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_values()) == [2]
+    assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_values()) == []
+
     # __next__ must continue to raise StopIteration
     it = m.IntPairs([(0, 0)]).nonzero()
     for _ in range(3):
@@ -55,6 +59,31 @@ def test_generalized_iterators_simple():
         (0, 5),
     ]
     assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_keys()) == [1, 3, 0]
+    assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_values()) == [2, 4, 5]
+
+
+def test_iterator_referencing():
+    """Test that iterators reference rather than copy their referents."""
+    vec = m.VectorNonCopyableInt()
+    vec.append(3)
+    vec.append(5)
+    assert [int(x) for x in vec] == [3, 5]
+    # Increment everything to make sure the referents can be mutated
+    for x in vec:
+        x.set(int(x) + 1)
+    assert [int(x) for x in vec] == [4, 6]
+
+    vec = m.VectorNonCopyableIntPair()
+    vec.append([3, 4])
+    vec.append([5, 7])
+    assert [int(x) for x in vec.keys()] == [3, 5]
+    assert [int(x) for x in vec.values()] == [4, 7]
+    for x in vec.keys():
+        x.set(int(x) + 1)
+    for x in vec.values():
+        x.set(int(x) + 10)
+    assert [int(x) for x in vec.keys()] == [4, 6]
+    assert [int(x) for x in vec.values()] == [14, 17]
 
 
 def test_sliceable():
@@ -160,6 +189,7 @@ def test_map_iterator():
         assert sm[k] == expected[k]
     for k, v in sm.items():
         assert v == expected[k]
+    assert list(sm.values()) == [expected[k] for k in sm]
 
     it = iter(m.StringMap({}))
     for _ in range(3):  # __next__ must continue to raise StopIteration