Task #2859
Change ArrayRef iterator type from pointer to std::iterator
Description
Currently a number of coding bugs are possible when comparing iterators from ArrayRef to those from other std::containers, as ArrayRef::iterator is actual a pointer to the data in the container.
Changing the iterator type would avoid bugs where those iterators are compared to pointers or nullptr, but would add complexity to the code.
Another option would be using std::span for ArrayRef itself but this would either need to be natively implemented or imported from an existing implementation.
Associated revisions
Change ArrayRef to use a proper iterator
Fixes #2859
History
#1 Updated by Paul Bauer almost 2 years ago
- Tracker changed from Feature to Task
I think this is better described as a task
#2 Updated by Mark Abraham almost 2 years ago
My suggestion is that we fix the ArrayRef iterator (I think that is easy). Then we should get a c++14 std::span like thing into compat, or similar. I did have some WIP on gerrit for a c++11 span some time ago, but by now we'd do better to import a new one.
#3 Updated by Gerrit Code Review Bot almost 2 years ago
Gerrit received a related patchset '1' for Issue #2859.
Uploader: Roland Schulz (roland.schulz@intel.com)
Change-Id: gromacs~master~I80e2b6a75bca0973f550e0ab8fa62c622c330f90
Gerrit URL: https://gerrit.gromacs.org/9109
#4 Updated by Roland Schulz almost 2 years ago
- Status changed from New to Fix uploaded
Instead of importing a new std::span we probably should explore making span a simple 1D template alias of mdspan. If we don't discover any significant disadvantage this would avoid duplication.
#5 Updated by Roland Schulz almost 2 years ago
We need to fix this for begin/end for mdspan too (added here). Independent of whether we use mdspan for span or not.
#6 Updated by Gerrit Code Review Bot almost 2 years ago
Gerrit received a related patchset '1' for Issue #2859.
Uploader: Roland Schulz (roland.schulz@intel.com)
Change-Id: gromacs~master~I0b61abdc2e60285a7ca17e3bdc7e53cb72c5cf6a
Gerrit URL: https://gerrit.gromacs.org/9119
#7 Updated by Roland Schulz 7 months ago
- Status changed from Fix uploaded to Resolved
Applied in changeset c95f8de2b627bb8e26e3fc29bf99722ec909b640.
Remove EmptyArrayRef
Make it more aligned with std::span.
Almost everywhere it wasn't needed anyhow.
Only exception: ternary conditional operator. But there the type
was already explict in all but one case and that one case
(src/gromacs/domdec/distribute.cpp) was confusing because of
multiple implicit conversions.
Related #2859
Change-Id: I0b61abdc2e60285a7ca17e3bdc7e53cb72c5cf6a