-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix sequtils.delete bug with out of bounds indexes #12506
Conversation
@@ -434,7 +434,9 @@ proc delete*[T](s: var seq[T]; first, last: Natural) = | |||
var dest = @[1, 1, 1, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1] | |||
dest.delete(3, 8) | |||
assert outcome == dest | |||
|
|||
doAssert first <= last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously sending first > last
resulted in a IndexError
, which doesn't tell the user much about what went wrong. If sending first > last
is supposed to work I can fix it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, first greater than last should silently delete nothing. This how you define empty sequence with inclusive ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And first >= len should be IndexError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And first >= len should be IndexError
I actually agree, but it would be inconsistent with how the last
parameter is handled (out of bounds allowed) and how system.delete
behaves. If first >= len
results in IndexError then last >= len
should also result in IndexError, which would be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some Python influence here too, "oobasfd"[:10000] == 'oobasfd'
, Python is usually silent about index out of bounds for slices.
The code example in #12388 shows that also Can you also patch that one? Btw, |
This improves the situation so I'm merging this now and later on we can make these APIs more consistent with each other. |
The proc already handled the case where
last
is out of bounds, so this is imo just a bugfix.Fixes #12388