fix: iterator increment operator does not skip first item#5400
fix: iterator increment operator does not skip first item#5400rwgk merged 2 commits intopybind:masterfrom
Conversation
|
This is a BC breaking change, right? |
|
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 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. |
|
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. |
|
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 |
|
As far as I'm concerned, it's ready for merging. |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
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.
henryiii
changed the title
4 participants
Description
Fixes #5399
I fixed this bug by ensuring that the increment operator calls
advance()one more time ifadvance()had never been called before (likeoperator*andoperator->already did). I encapsulated this logic in a helper functioninit().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 asoperator*andoperator->already did.Suggested changelog entry: