* Add a test showing a flaw in make_key_iterator/make_value_iterator
If the iterator dereference operator returns a value rather than a
reference (and that pair also does not *contain* references),
make_key_iterator and make_value_iterator will return a reference to a
temporary, causing a segfault.
* Fix make_key_iterator/make_value_iterator for prvalue iterators
If an iterator returns a pair<T1, T2> rather than a reference to a pair
or a pair of references, make_key_iterator and make_value_iterator would
return a reference to a temporary, typically leading to a segfault. This
is because the value category of member access to a prvalue is an
xvalue, not a prvalue, so decltype produces an rvalue reference type.
Fix the type calculation to handle this case.
I also removed some decltype parentheses that weren't needed, either
because the expression isn't one of the special cases for decltype or
because decltype was only used for SFINAE. Hopefully that makes the code
a bit more readable.
Closes#3347
* Attempt a workaround for nvcc
* 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>
* Adding nullptr slices
Using example from #1095
Some fixes from @wjakob's review
Stop clang-tidy from complaining
New proposal for py::slice constructor
Eric's suggested changes: simplify testing; shift def's
* chore: drop MSVC pragma (hopefully unneeded)
* Apply suggestions from code review
* 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
* Apply isort
* Tweak isort config
* Add env.py as a known_first_party
* Add one missing known first party
* Make config compat with older isort versions
* Add another comment
* Revert pyproject setting
The just-updated flake8 package hits a bunch of:
E741 ambiguous variable name 'l'
warnings. This commit renames them all from `l` to `lst` (they are all
list values) to avoid the error.
This udpates all the remaining tests to the new test suite code and
comment styles started in #898. For the most part, the test coverage
here is unchanged, with a few minor exceptions as noted below.
- test_constants_and_functions: this adds more overload tests with
overloads with different number of arguments for more comprehensive
overload_cast testing. The test style conversion broke the overload
tests under MSVC 2015, prompting the additional tests while looking
for a workaround.
- test_eigen: this dropped the unused functions `get_cm_corners` and
`get_cm_corners_const`--these same tests were duplicates of the same
things provided (and used) via ReturnTester methods.
- test_opaque_types: this test had a hidden dependence on ExampleMandA
which is now fixed by using the global UserType which suffices for the
relevant test.
- test_methods_and_attributes: this required some additions to UserType
to make it usable as a replacement for the test's previous SimpleType:
UserType gained a value mutator, and the `value` property is not
mutable (it was previously readonly). Some overload tests were also
added to better test overload_cast (as described above).
- test_numpy_array: removed the untemplated mutate_data/mutate_data_t:
the templated versions with an empty parameter pack expand to the same
thing.
- test_stl: this was already mostly in the new style; this just tweaks
things a bit, localizing a class, and adding some missing
`// test_whatever` comments.
- test_virtual_functions: like `test_stl`, this was mostly in the new
test style already, but needed some `// test_whatever` comments.
This commit also moves the inherited virtual example code to the end
of the file, after the main set of tests (since it is less important
than the other tests, and rather length); it also got renamed to
`test_inherited_virtuals` (from `test_inheriting_repeat`) because it
tests both inherited virtual approaches, not just the repeat approach.
Fixes#896.
From Python docs: "Once an iterator’s `__next__()` method raises
`StopIteration`, it must continue to do so on subsequent calls.
Implementations that do not obey this property are deemed broken."
Before this, `py::iterator` didn't do any error handling, so code like:
```c++
for (auto item : py::int_(1)) {
// ...
}
```
would just silently skip the loop. The above now throws `TypeError` as
expected. This is a breaking behavior change, but any code which relied
on the silent skip was probably broken anyway.
Also, errors returned by `PyIter_Next()` are now properly handled.
A flake8 configuration is included in setup.cfg and the checks are
executed automatically on Travis:
* Ensures a consistent PEP8 code style
* Does basic linting to prevent possible bugs
Use simple asserts and pytest's powerful introspection to make testing
simpler. This merges the old .py/.ref file pairs into simple .py files
where the expected values are right next to the code being tested.
This commit does not touch the C++ part of the code and replicates the
Python tests exactly like the old .ref-file-based approach.
