Project

General

Profile

Task #2859

Change ArrayRef iterator type from pointer to std::iterator

Added by Paul Bauer 2 months ago. Updated 2 months ago.

Status:
Fix uploaded
Priority:
Normal
Assignee:
-
Category:
core library
Target version:
-
Difficulty:
hard
Close

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

Revision 8b901fad (diff)
Added by Roland Schulz 2 months ago

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

History

#1 Updated by Paul Bauer 2 months ago

  • Tracker changed from Feature to Task

I think this is better described as a task

#2 Updated by Mark Abraham 2 months 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 2 months ago

Gerrit received a related patchset '1' for Issue #2859.
Uploader: Roland Schulz ()
Change-Id: gromacs~master~I80e2b6a75bca0973f550e0ab8fa62c622c330f90
Gerrit URL: https://gerrit.gromacs.org/9109

#4 Updated by Roland Schulz 2 months 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 2 months 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 2 months ago

Gerrit received a related patchset '1' for Issue #2859.
Uploader: Roland Schulz ()
Change-Id: gromacs~master~I0b61abdc2e60285a7ca17e3bdc7e53cb72c5cf6a
Gerrit URL: https://gerrit.gromacs.org/9119

Also available in: Atom PDF