Skip to content
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

sequtils.delete and system.delete inconsistencies when index out of bounds #12388

Closed
koranza opened this issue Oct 8, 2019 · 0 comments · Fixed by #12506
Closed

sequtils.delete and system.delete inconsistencies when index out of bounds #12388

koranza opened this issue Oct 8, 2019 · 0 comments · Fixed by #12506

Comments

@koranza
Copy link
Contributor

koranza commented Oct 8, 2019

The delete proc for seqs is defined in system and sequtils.

The difference is that the definition from system takes 1 parameter for the index to delete whereas the definition from sequtils takes 2 parameters which represent the range of indices to delete (inclusive)

Since the range is inclusive, you would expect both results below to be the same

Example

import sequtils

var ones: seq[string] 

echo "-- delete at index 50 ---"

ones = @["one"]
ones.delete(50)           # unexpected: shouldn't delete anything since index 50 is out of bounds
echo repr ones

ones = @["one"]
ones.delete(50,50)        # unexpected: should be the same thing as delete(50)
echo repr ones

Current Output

-- delete at index 50 ---
@[]
@["one", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""]

Expected Output

@["one"]
@["one"]

Suggestions

Make deleting an out of bounds index return the same exact array, unmodified. (Or possibly throw an error)
Possibly make the proc return a bool if the array was modified using the discardable pragma so that its ignored by default

The documentation should say what happens for edge cases like when a parameter is out of bounds for the seq

Additional Information

$ nim -v
Nim Compiler Version 0.20.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants