Fix #5399: iterator increment operator does not skip first item

[dalboris]

Description

Fixes #5399

I fixed this bug by ensuring that the increment operator calls advance() one more time if advance() had never been called before (like operator* and operator-> already did). I encapsulated this logic in a helper function init().

Alternatively, the init() function could directly be called in the constructor, which would make more sense and be more performant since it avoids the extra check on all operators. But I'm not sure how to do this since the constructor is defined via a macro. This is why I used the same method as operator* and operator-> already did.

Suggested changelog entry:

* Fix iterator increment operator does not skip first item

[Skylion007]

This is a BC breaking change, right?

[dalboris]

Yes, it is a breaking change, but I doubt anyone was relying on the previous behavior, since it was clearly broken. Generic C++ algorithm or ranges-view may fail with the current implementation. In my case, I was using drop(range, 1) which basically did nothing: the returned range still included the first item.

Note: this shouldn't be merged just yet as I realized another problem: init() should be called first in the post-fix increment. I'm adding a test for that too.

[dalboris]

Oh, my bad, I guess you're asking about binary compatibilty. No, I do not think this change breaks binary compatibility, as it does not add any member variables. It only adds a private member method and changes the implementation of the existing public methods.

[dalboris]

So just to clarify, the values before this change were:

def test_iterable(doc):
    assert doc(m.get_iterable) == "get_iterable() -> Iterable"
    lins = [1, 2, 3]
    i = m.get_first_item_from_iterable(lins)
    assert i == 1 # Before this fix, this failed with i being equal to 3 instead
    i = m.get_second_item_from_iterable(lins)
    assert i == 2 # Before this fix, this failed with i being equal to 1 instead

[dalboris]

As far as I'm concerned, it's ready for merging.

[rwgk]

Thanks for the fix!

Pretty amazing that nobody complained before.

I convinced myself that this is correct by locally adding more tests, but I don't think we need to add them here. What you have appears to be to the point.

@Skylion007 could you please confirm that your "BC breaking change" concerns are addressed?

To me this looks like a regular plain bug fix.