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

[BUG]: error: lvalue required as increment operand #4100

Closed
2 of 3 tasks
hiaselhans opened this issue Jul 30, 2022 · 9 comments · Fixed by #4234
Closed
2 of 3 tasks

[BUG]: error: lvalue required as increment operand #4100

hiaselhans opened this issue Jul 30, 2022 · 9 comments · Fixed by #4234
Labels

Comments

@hiaselhans
Copy link

Required prerequisites

Problem description

previously compiling code fails with gcc 12.1.0:

/usr/include/pybind11/pybind11.h:2347:29: error: lvalue required as increment operand

relevant code:

iterator make_iterator_impl(Iterator &&first, Sentinel &&last, Extra &&...extra) {
using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
// TODO: state captures only the types of Extra, not the values
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 {
if (!s.first_or_done) {
++s.it;
} else {
s.first_or_done = false;
}
if (s.it == s.end) {
s.first_or_done = true;
throw stop_iteration();
}
return Access()(s.it);
// NOLINTNEXTLINE(readability-const-return-type) // PR #3263
},
std::forward<Extra>(extra)...,
Policy);
}

Reproducible example code

#include <pybind11/pybind11.h>
#include <vector>
#include <pybind11/stl.h>

namespace py = pybind11;
using namespace py::literals;

class Vec {
    public:
        Vec(double x, double y, double z) {
            values[0] = x;
            values[1] = y;
            values[2] = z;
        };
        double values[3];
};

PYBIND11_MODULE(mymodule, m) {
    py::class_<Vec>(m, "Vec")
        .def(py::init<double, double, double>())
        .def("__iter__", [](const Vec& v){
            return py::make_iterator(v.values, v.values+3);
        },  py::keep_alive<0, 1>());
            
}
@hiaselhans hiaselhans added the triage New bug, unverified label Jul 30, 2022
@Skylion007
Copy link
Collaborator

@hiaselhans Does this code snippit work with pybind11 2.9?

@Skylion007 Skylion007 added bug and removed triage New bug, unverified labels Aug 1, 2022
@Simon-Lopez
Copy link

Hello,

Don't know if it helps but the code above compiles fine replacing:

        return py::make_iterator(v.values, v.values + 3);

with:

        return py::make_iterator(v.values);

(wich will then look for begin and end of v.values which is of type const double[3]) or casting to raw pointer type with:

        return py::make_iterator(static_cast<const double*>(v.values), v.values + 3);

or:

        return py::make_iterator(&(v.values[0]), &(v.values[0]) + 3);

to select th right overloaded function.

@Simon-Lopez
Copy link

It seems that the introduction of forwarding reference in 2.10.0 for:

iterator make_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra)

tends to favor the selection of:

iterator make_iterator(Type &value, Extra &&...extra) 

which leads to the "bug" above.

@Simon-Lopez
Copy link

Could #4105 be related to this ?
(as the signature of py::make_iterator is changed ?)

@Skylion007
Copy link
Collaborator

Skylion007 commented Oct 7, 2022

@rwgk @henryiii reverting the perfect forwarding looks like a safe change as First and List is always an l-value in the C++ STL. @henryiii This seems to be similar the bug you have found in one of your projects when upgrading to 2.10?

@henryiii
Copy link
Collaborator

henryiii commented Oct 7, 2022

Not me, but the regressions others found I listed in #4125. The regression for me has been the scikit_build_example one, which I need to work out before a release. The overload one is likely similar.

@rwgk
Copy link
Collaborator

rwgk commented Oct 7, 2022

Could #4105 be related to this ? (as the signature of py::make_iterator is changed ?)

I think it's almost certainly a different kind of breakage.
The first major difference is compilation error vs runtime error.

@rwgk
Copy link
Collaborator

rwgk commented Oct 7, 2022

@rwgk @henryiii reverting the perfect forwarding looks like a safe change as First and List is always an l-value in the C++ STL.

This one? https://github.com/pybind/pybind11/pull/3860/files

Sounds good to me: I'm unable to foresee the side-effects one way or another (so many details come into play), but the older version is time-tested. Including something like the test in the PR description here with the rollback seems important.

@Simon-Lopez
Copy link

Simon-Lopez commented Oct 10, 2022

Could #4105 be related to this ? (as the signature of py::make_iterator is changed ?)

I think it's almost certainly a different kind of breakage. The first major difference is compilation error vs runtime error.

I was just asking because I had runtime errors due to this very change. I did not investigate much further. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants