dsharlet / array

C++ multidimensional arrays in the spirit of the STL
Apache License 2.0
198 stars 15 forks source link

Use non-resolving version of array_ref::array_ref where appropriate #98

Closed dsharlet closed 10 months ago

dsharlet commented 10 months ago

This fixes an issue I noticed with @fbleibel-g where calls to resolve_unknown_strides was getting called in an example where I didn't expect it. This modifies some uses of array_ref::array_ref to use the non-resolving version of the constructor when we're using a shape that should have already been resolved.

This is a bit of a dangerous change, it's possible that some wonky usage of the array library could result in unresolved shapes getting resolved via these calls, but IMO that would be a bug if that were the case, that should be fixed elsewhere.

fbleibel-g commented 10 months ago

It would be nice to have type-safety around resolved shapes so we don't need to worry about future mistakes. Something like:

class resolved_shape<Shape> { Shape s; }

class array_ref {
  array_ref(T* base, const Shape& shape) {
    resolved_shape_ = shape.resolve();
  }
  array_ref(T* base, const resolved_shape<Shape>& shape) : resolved_shape_(shape) {
  }
  Shape& shape() { return resolved_shape_.shape(); }
}

etc. But that might be a somewhat viral change.

dsharlet commented 10 months ago

That's an interesting idea, but I'm a bit wary because "resolved-ness" isn't a static property. Nothing would stop me from saying resolved_shape.dim<1>().set_stride(dynamic), and now that "resolved" shape isn't resolved any more.

fbleibel-g commented 10 months ago

I see, and anyone can modify the shape of an arrayref in-flight with `Shape& shape() const { return shape; }`, though I think the use cases for this are slim. It would be nicer if this was immutable or a resolved shape (type wise). Anyway, food for thought.