Convenience constructor templates for buffer_info

[YannickJadoul]

In order to reduce the amount of error-prone constructor arguments, buffer_info would now have a constructor template similar to array to automatically get the item size, default format string and number of dimensions.

[dean0x7d]

That's a nice reduction of arguments. Looks good to me.

[YannickJadoul]

Somehow the builds are failing on gcc 4.8, though. gcc 6 and clang do work. And on my own system (gcc 5.4), I had to use brace-initialization.

For some reason, it seems that the compiler first moves the arguments to the other constructor, and only then gets the size() of the shape vector.
Or, of course, I'm missing something else and there's other undefined behaviour...

[dean0x7d]

I think the frequent false positives on AppVeyor have desensitized me to seeing red in CI status. Totally missed the gcc 4.8 error.

[dean0x7d]

It's an argument evaluation order issue. shape_in is being used and moved out of in the same call -- undefined behavior. GCC >5 and clang evaluate shape_in->size() before std::move(shape_in)but the order isn't specified by the C++ standard.Edit: Didn't see the brace initialization.

[YannickJadoul]

I'd agree, but there's two things why I'm not completely convinced:

• First, std::move should be nothing more than a cast to an rvalue reference. The actual move should only happen at the construction time of the callee's arguments, no?
• Second, the brace-initialization dóes have guaranteed ordering according to the C++11 standard.

[YannickJadoul]

Ah, but, stack overflow knows about a gcc bug: https://stackoverflow.com/questions/14060264/order-of-evaluation-of-elements-in-list-initialization
So maybe I'm wrong about that first point, then.

Edit: another interesting StackOverflow thread: https://stackoverflow.com/questions/24814696/move-semantics-and-function-order-evaluation

[dean0x7d]

Regarding point 1, each argument is fully evaluated before moving on to the next one, so the rvalue-reference cast and move-constructor call happen back-to-back.

MSVC has the same bug as gcc 4.8 regarding brace evaluation order. AFAIK this only affects calling constructors with braces, but not std::initializer_list and aggregates.

Unless someone has a better solution, I think making a copy of shape_in might be an acceptable workaround here.

[jagerman]

I ran into exactly the same issue in numpy.h when implementing this there in the array classes, and copying was what I went with. So if someone finds a better solution, apply it there too! worked around it.

[YannickJadoul]

Let's see... *fingers crossed*
I've implemented this suggestion from StackOverflow, basically adding an intermediate private constructor that takes an rvalue reference instead. This way, the strides_in variable doesn't get moved until this private constructor runs (and by that time, we have surely queried its size).

Edit: woohoo, GCC 4.8 already seems to have succeeded. Should I also implement this solution for array in this same PR, or ... ?

[jagerman]

Is there any pointer in declaring the default constructor here? (I.e. instead of just struct private_ctr_tag {};?)

[YannickJadoul]

I think I saw that somewhere, and reckoned explicit would be appropriate for the constructor. But there's actually no point, since it's all private, anyway.

[jagerman]

Edit: woohoo, GCC 4.8 already seems to have succeeded. Should I also implement this solution for array in this same PR, or ... ?

Actually, I don't think it's needed anymore; I forget exactly where it happened, but it looks like the current array implementation just moves (and just gets ndim from shape->size() rather than passing it separately).

[YannickJadoul]

Actually, I don't think it's needed anymore; I forget exactly where it happened, but it looks like the current array implementation just moves (and just gets ndim from shape->size() rather than passing it separately).

Is there a reason for these two interfaces (buffer_info and array) to differ from each other? It feels like they're pretty similar in function. Things like e.g. default strides could also be useful for creating a buffer_info?

[YannickJadoul]

Oh, also, on the topic of default_strides: I noticed that the current implementation

std::fill(strides.begin(), strides.end(), itemsize);
for (size_t i = 0; i < ndim - 1; i++)
for (size_t j = 0; j < ndim - 1 - i; j++)
strides[j] *= shape[ndim - 1 - i];

could be simplified to the more efficient

strides[ndim - 1] = itemsize;
for (size_t i = 1; i < ndim; ++i)
strides[ndim - 1 - i] = strides[ndim - i] * shape[ndim - i];

If we leave array alone in this PR, this change could be a different PR (or none at all, if it's not worth the change), but I thought to just mention it.

[jagerman]

could be simplified to the more efficient

Already pending in #762.

[jagerman]

I need the same trick in #762, so how about moving the struct private_ctr_tag {}; into the pybind11::detail namespace in common.h. (Also minor nitpick: let's rename to private_ctor_tag {};, using the more common constructor abbreviation).

[jagerman]

Actually, never mind that. I'll move it as part of #762; you can leave this PR as-is.

[jagerman]

Perhaps this could be redesigned a bit to only use this private constructor when you need the delegation, i.e. just in the one constructor at line 34 that wants to both move and call ->size. It makes the code a little easier to follow (one fewer delegation for most constructors), and invokes fewer moves. It's also essentially a workaround for MSVC and old gcc, rather than a particularly desirable design.

[YannickJadoul]

This is what you mean?

This costs one extra move when you use the buffer_info(T *ptr, detail::any_container<ssize_t> shape_in, detail::any_container<ssize_t> strides_in) overload (as the private constuctor forwards it to the original overload and that one takes the containers by value before moving it into the data members, while before, the private constructor moved it immediately into the private members).
Then again, even if the optimizer doesn't figure that out, that extra move won't be thát costly, of course.

[jagerman]

Oh, right, it's not actually fewer delegations. Still, I like the idea of keeping the workaround as out-of-the-way as possible rather than pushing everything through it.

[dean0x7d]

Merged, thanks @YannickJadoul!