pybind::array and pybind::array_t universal references

[SylvainCorlay]

Variadic template methods of pybind::array and pybind::array_t used for accessing data, namely

data, mutable_data, offset_at, index_at, get_byte_offset , at, mutable_at

should take arguments by value instead of universal reference.

The semantic in the standard library for functions taking integral or iterator arguments is to take them by value. (For example std::vector<T>::operator[] takes its argument by value).

[SylvainCorlay]

^@aldanor

[jagerman]

The issue is that, although all arguments get converted to size_t eventually, the variable-arguments-through-a-template-parameter-pack approach being used here doesn't allow that implicit conversion to take place.

For stl functions like vector's operator[] I get implicit conversion because the argument is a size_t, which means that I can call it with anything that is implicitly convertible to size_t, e.g.:

class SomeNonCopyableClass {
    // ...
    operator size_t() const { return /* ... */; }
};

// ...

SomeNonCopyableClass a(2);
std::cout << myvec[a] << "\n";

With pass-by-value semantics without the type being in the function signature, this fails because I can't copy my a object to pass it along: the deduced argument type will be SomeNonCopyableClass, not size_t. But the current code fails here, too, because the various argument chains actually are passing by value rather than forwarding arguments. This should be fixed.

So I think, to have an stl-like interface that takes anything convertible to a size_t, the code should keep the && arguments, but needs to also use perfect forwarding, e.g. by changing the calls such as next_method(index...) to either next_method(std::forward<Ix>(index)...) (to pass the conversion down the line), or next_method(size_t(index)...) to do the conversion immediately.

On a side note: there's an index_at method that current takes its arguments by lvalue reference, which looks like a typo (i.e. it's missing the second &).

[SylvainCorlay]

So I think, to have an stl-like interface that takes anything convertible to a size_t, the code should keep the && arguments, but needs to also use perfect forwarding, e.g. by changing the calls such as next_method(index...) to either next_method(std::forward(index)...) (to pass the conversion down the line), or next_method(size_t(index)...) to do the conversion immediately.

Can you think of a realistic case where people would be indexing with non-integral types convertible to size_t?

[jagerman]

Sure; perhaps I encode something else in my indices--a row title, for instance--and so my index objects are:

struct myindex {
    size_t i;
    std::string name;
    operator size_t() const { return i; }
};

Sure, there are other ways I could do this, but what is the compelling reason to not allow arbitrary types that are implicitly convertible to size_t to be used? (In other words, what does pass-by-value gain over perfect forwarding?)

[SylvainCorlay]

Sure, there are other ways I could do this, but what is the compelling reason to not allow arbitrary types that are implicitly convertible to size_t to be used? (In other words, what does pass-by-value gain over perfect forwarding?)

Not much except that it is the most used semantic for integral types, so universal references go against the principle of least surprise.

[SylvainCorlay]

In some unlikely cases it can have border effects, such as an object being moved unexpectedly. For example if you have a function foo that transform the index, but sometimes only returns it unchanged as an rvalue (effectively equivalently to std::move). Then myvec[foo(i)] will cause i to be moved and not usable afterwards.

[SylvainCorlay]

Regarding your concern regarding non-copyable things, I think that it is a good assumptions / requirement for and index to have a value semantic.

[jagerman]

I agree: the ideal interface here is (size_t i_0, size_t i_1, size_t i_2, ...) where the number of arguments is variable. But I don't see an easy way to do that here (two things that occur to me: making each of the calls a recursive template so that all arguments really do have to be size_t; or using va_args). Neither is all that nice, but perhaps one or the other is worth doing anyway.

[dean0x7d]

In some unlikely cases it can have border effects, such as an object being moved unexpectedly. For example if you have a function foo that transform the index, but sometimes only returns it unchanged as an rvalue. Then myvec[foo(i)] will cause i to be moved and not usable afterwards.

That would be a fault in foo, not in myvec::operator[]. There's really no way to accidentally move out an lvalue -- std::move is required and very loud. (Pure rvalues are moved silently, but there's nothing unexpected about that.)

The only downside of forwarding references is that they result in extra overhead (pointers to ints) in case the template function isn't inlined (unlikely for short templates, but it does happen). There's no difference for fully inlined templates.

It would probably be best to keep things simple and optimize for the most common use case. These are pretty much always going to be simple integers so value semantics seems preferable.

[JohanMabille]

@dean0x7d said:
It would probably be best to keep things simple and optimize for the most common use case. These are pretty much always going to be simple integers so value semantics seems preferable.

I strongly agree with @SylvainCorlay and @dean0x7d on that; passing by value is preferable, and follows the conventions of the C++ standard library.

@jagerman said:
I agree: the ideal interface here is (size_t i_0, size_t i_1, size_t i_2, ...) where the number of arguments is variable. But I don't see an easy way to do that here (two things that occur to me: making each of the calls a recursive template so that all arguments really do have to be size_t; or using va_args). Neither is all that nice, but perhaps one or the other is worth doing anyway.

The recursive approach is fine, it is what we use in xtensor. It is more efficient, no additional array on the stack is required, and the compiler can inline it so the assembly is just exactly the computation of the index in the underlying buffer, nothing more. And you force conversion to size_t for free.

[SylvainCorlay]

@wjakob what do you think of the above?

[wjakob]

It sounds like a fairly low level change -- either is fine with me I guess. This piece of code "belongs" to @aldanor, who has done an excellent job designing these APIs -- so I ultimately defer to him.

[SylvainCorlay]

There is a significant performance difference too.

Regarding the dimension checks, I would like to remove them all and only check at the top-level calls to at and data.

[dean0x7d]

Significant performance difference for what?

Are the checks being duplicated somewhere or do you just want to get rid of them altogether? The latter was discussed previously and went in favor of checking.