BUG: Fix reverse iterator increment return type

[maekclena]

Fixes #775.

[blowekamp]

Please add a test to verify what you have written has correct behavior. The issue reported has a test which can be expanded.

[N-Dekker]

Nice to have a fix for the old ReverseIterator, and have the return value of operator++ properly tested. Thanks @maekclena .

Still I hope that FixedArray::ReverseIterator can eventually be removed from ITK. @phcerdan Wasn't your idea to eventually remove the old ReverseIterator, when you suggested me to write a new one, for FixedArray, at #757 (comment) ?

[phcerdan]

Still I hope that FixedArray::ReverseIterator can eventually be removed from ITK. @phcerdan Wasn't your idea to eventually remove the old ReverseIterator, when you suggested me to write a new one, for FixedArray, at #757 (comment) ?

No, it wasn't my idea and the first time I hear that removing ReverseIterator is desired. In #757 you were adding compatibility of FixedArray to the std algorithms adding begin(), end() et al (which was great!), and I asked to be more feature complete adding rbegin and rend.

What is really desired is to fix bugs, so +100 to this PR and @maekclena!

[N-Dekker]

I have to check again... shouldn't operator++() return a reference to the iterator itself?

[N-Dekker]

...

[N-Dekker]

@dzenanz I still had a technical question about this PR: #850 (review) So why did you already merge?

[N-Dekker]

@maekclena For your information, this example code still fails now, even with your fix being merged:

auto newReverseIterator = fixedArray.rbegin();
auto oldReverseIterator = fixedArray.rBegin();

// Move two steps in reverse direction!
++++oldReverseIterator;
++++newReverseIterator;

EXPECT_EQ(*oldReverseIterator, *newReverseIterator);

[dzenanz]

@N-Dekker this PR did what its title says "Fix ... return type". All the increment/decrement operators return ReverseIterators so they cannot return Iterators. And your comment ... was a bit confusing, I thought you figured out the question from previous comment. Hence I merged it. Sorry for misunderstanding.

You could expand and reopen the issue, which might get addressed in another PR. Which you might attempt yourself?

[N-Dekker]

@dzenanz First of all, I do agree that the PR as it is merged now does already improve the behavior of the old reverse iterator. But then again, it still does not seem interesting to me to keep maintaining the old FixedArray::ReverseIterator. I think users should just switch to the new one!

Advantages of the new FixedArray::reverse_iterator:

1. Can be passed directly to std algorithms, for example std::sort(fixedArray.rbegin(), fixedArray.rend()), which won't compile for the old rBegin() and rEnd()
2. Supports random access iteration (while the old ReverseIterator only supports bidirectional iteration)
3. Has a base() function to return the original (forward) iterator
4. Takes less maintenance from ITKdevelopers, as it just uses the std::reverse_iterator implementation.

The old FixedArray::ReverseIterator does not seem to offer anything extra, does it?

[dzenanz]

Can you make a PR which deprecates it?

[N-Dekker]

Can you make a PR which deprecates it?

I would like to, thanks @dzenanz, but isn't it better now to postpone such deprecation until after the release of ITK5?

[dzenanz]

If we are going to deprecate it soon, before the release is probably better than after.