Skip to content

Fix double dot range for invalid input #164

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

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Conversation

shmsr
Copy link
Contributor

@shmsr shmsr commented Feb 18, 2021

For double dot range i.e., a..b where a and b are inclusive when ranging from a to b. Therefore every element x in the range must satisfy a <= x && x <= b

Previously, it used to panic because, for a..b, a is considered minimum and b is considered maximum which results in negative size when allocating memory for slice using make. Ref:

file: optimizer/const_range.go

size := max.Value - min.Value + 1
...
...
value := make([]int, size)

So, for invalid inputs where a > b, it panics. Similar problem is there in vm/runtime.go's makeRange function as well.

Changing it in the optimizer and makeRange seems apt to me, but I figured it could be handled in checker/checker.go and cry the error out loud that the expression is not correct, but it didn't feel right.

@antonmedv What do you think? Having an empty slice instead of exiting out with an error seems good, right?

Fixes #163

@shmsr shmsr changed the title Fix double dot range for invalid input (Issue #163) Fix double dot range for invalid input Feb 19, 2021
@shmsr shmsr mentioned this pull request Feb 25, 2021
@johejo
Copy link

johejo commented Feb 26, 2021

Could you add tests to fix #163 ?

@shmsr
Copy link
Contributor Author

shmsr commented Feb 26, 2021

@johejo done!

@antonmedv
Copy link
Member

I think same way work in python. So I think it make sense.

@shmsr
Copy link
Contributor Author

shmsr commented Feb 27, 2021

I think same way work in python. So I think it make sense.

So, this PR looks good, right? Could be merged?

@antonmedv
Copy link
Member

Yes, gonna test it today and merge.

@antonmedv antonmedv merged commit 2c1881a into expr-lang:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic with invalid input
3 participants