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

Fixes 9880 #10228

Closed
wants to merge 8 commits into from
Closed

Fixes 9880 #10228

wants to merge 8 commits into from

Conversation

BontaVlad
Copy link

@BontaVlad BontaVlad commented Jan 7, 2019

Fixes #9880
I'm still learning nim, of course this PR needs adult supervision.

@krux02
Copy link
Contributor

krux02 commented Jan 7, 2019

Can you add a test case please? Apart from that, I agree, the error should contain the bounds.

@narimiran
Copy link
Member

I'm still learning nim, of course this PR needs adult supervision.

Your contributions (I'm including here your comment on #10198, which led to a pull-request and a fix) are very appreciated and welcome. Keep them coming as you learn Nim.

@narimiran
Copy link
Member

From my understanding (and I might be wrong — please correct me), formatErrorIndexBound returns a valid index range (usually from 0 to len-1), which would mean that in several places where you pass someLength+1 you should pass someLength-1.

(Don't correct anything until somebody confirms this :))

@krux02
Copy link
Contributor

krux02 commented Jan 7, 2019

@narimiran A test case that triggers these exceptions should clear that question up.

@BontaVlad
Copy link
Author

I'm on board with having tests, I just need some directions on how do I trigger these exceptions and also how to cache the mentioned exceptions.

@krux02
Copy link
Contributor

krux02 commented Jan 8, 2019

you should be able to raise the exception when you acces an arrar or a seq outside of its bounds. With try catch you can catch the exception and write an assert for the message.

@BontaVlad
Copy link
Author

/cc @narimiran @krux02. I fixed the bounds, also I tried to add tests in the parts that I understood how to trigger the exceptions, for the others I tested with either echo commands or gdb, or common sense. If you know how to trigger the missing ones or you know how to play with vm.nim optcode better than nimscript feel free to ping me.

@timotheecour
Copy link
Member

@BontaVlad failure on appveyor is unrelated to your PR, it's caused by #10359

lib/system/jssys.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

friendly ping @BontaVlad

@BontaVlad
Copy link
Author

What do I need to do on this PR to get it merged?

@Araq
Copy link
Member

Araq commented Jan 31, 2019

Don't use a generic compilerproc, follow what the compilerproc for the C backend does.

timotheecour added a commit to timotheecour/Nim that referenced this pull request Feb 9, 2019
timotheecour added a commit to timotheecour/Nim that referenced this pull request Feb 9, 2019
timotheecour added a commit to timotheecour/Nim that referenced this pull request Feb 9, 2019
timotheecour added a commit to timotheecour/Nim that referenced this pull request Feb 9, 2019
timotheecour added a commit to timotheecour/Nim that referenced this pull request Feb 9, 2019
Araq pushed a commit that referenced this pull request Feb 10, 2019
* Make index out of bounds more useful by including the 'bounds'.
* fixes #9880 index out of bounds (remaining cases); revives #10228
* change err msg to: `index 3 not in 0 .. 1`
@BontaVlad
Copy link
Author

Since #10610 was merged, can I close this PR?

@timotheecour
Copy link
Member

timotheecour commented Feb 10, 2019

@BontaVlad yes, I don't think there's any remaining items left out; thanks again!

Araq pushed a commit that referenced this pull request Feb 18, 2019
* Make index out of bounds more useful by including the 'bounds'.
* fixes #9880 index out of bounds (remaining cases); revives #10228
* change err msg to: `index 3 not in 0 .. 1`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants