pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

[BUG]: error: lvalue required as increment operand

Open hiaselhans opened this issue 3 years ago • 1 comments

Required prerequisites

  • [X] Make sure you've read the documentation. Your issue may be addressed there.
  • [X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
  • [ ] Consider asking first in the Gitter chat room or in a Discussion.

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: https://github.com/pybind/pybind11/blob/1e3400b6742288429f2069aaf5febf92d0662dae/include/pybind11/pybind11.h#L2336-L2360

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 avatar Jul 30 '22 17:07 hiaselhans

@hiaselhans Does this code snippit work with pybind11 2.9?

Skylion007 avatar Aug 01 '22 17:08 Skylion007

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 avatar Oct 06 '22 20:10 Simon-Lopez

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 avatar Oct 06 '22 20:10 Simon-Lopez

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

Simon-Lopez avatar Oct 07 '22 13:10 Simon-Lopez

@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?

Skylion007 avatar Oct 07 '22 15:10 Skylion007

Not me, but the regressions others found I listed in https://github.com/pybind/pybind11/issues/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.

henryiii avatar Oct 07 '22 16:10 henryiii

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 avatar Oct 07 '22 16:10 rwgk

@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.

rwgk avatar Oct 07 '22 16:10 rwgk

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. :-/

Simon-Lopez avatar Oct 10 '22 08:10 Simon-Lopez