-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ArrayView::size()
should return ArrayView::size(0)
#253
Comments
I remember seeing these in GEOSX and I agree they're pretty ugly.
There is actually a
There are plenty of valid use cases, such if you want to zero out an array or add a scalar or search... Plus it's more efficient to iterate over in this manner. But I agree most of the time you'll need/want all the indices.
100% on board with this, I'm quite sure it's not used anywhere in GEOSX or anywhere else. Plus it would clean up the resizing logic.
I'm on board with this change. I think a decent way to go about doing it would be to as you suggest make the no argument |
Currently
ArrayView::size()
return the total number of elements across all dimensions. This creates a few inconveniences:Array<T,2>
andArrayOfArrays
that may represent connectivity maps between different kinds of mesh objects), one cannot uniformly extract the primary dimension size withmap.size()
. We have to provide workarounds like various overloaded helper functions, whereas an element can be accessed uniformly withmap(i,j)
.ArrayView
can be viewed as a range of lower-dimensional slices, howeversize()
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 providebegin()
andend()
iterators forNDIM > 1
, but I believe that can be fixed too).I think it would be more consistent behavior if
size()
returned the same value assize(0)
(orsize(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 functionnumElements()
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.The text was updated successfully, but these errors were encountered: