LvArray icon indicating copy to clipboard operation
LvArray copied to clipboard

`ArrayView::size()` should return `ArrayView::size(0)`

Open klevzoff opened this issue 2 years ago • 1 comments

Currently ArrayView::size() return the total number of elements across all dimensions. This creates a few inconveniences:

  • When dealing with various containers generically (e.g. Array<T,2> and ArrayOfArrays that may represent connectivity maps between different kinds of mesh objects), one cannot uniformly extract the primary dimension size with map.size(). We have to provide workarounds like various overloaded helper functions, whereas an element can be accessed uniformly with map(i,j).
  • An ArrayView can be viewed as a range of lower-dimensional slices, however size() does not return the size of that range (except in 1D case), and thus semantically it does not satisfy sized_range concept, so it may not be used with many of the C++ ranges algorithms (there are other problems as well, since we don't provide begin() and end() iterators for NDIM > 1, but I believe that can be fixed too).
  • On the other hand, interpreting a mutidimensional array as a range of all of its individual elements across all dimensions seems pointless, since it loses important semantics of dimensionality. I've not been able to find such use cases (which doesn't mean they don't exist, but I'd be curious to know).

I think it would be more consistent behavior if size() returned the same value as size(0) (or size(m_singleParameterResizeIndex), but in my opinion that member is useless and should just be removed). For use cases where the total number of elements is needed (e.g. allocating some buffer for serialization), a new member function numElements() could be introduced.

Since this is a subtle breaking change, it should be done with care. It's difficult to search for uses of .size() on arrays only in codebases, since it's called many times on all kinds of objects (vector, map, ArrayOfArrays, Array<T,1> for which there is no change, etc., and IDEs show too many false positives). So an agreement from all interested parties would be required.

klevzoff avatar Mar 24 '22 04:03 klevzoff