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 Parser.throwStatement() #510

Merged
merged 2 commits into from Jan 22, 2019
Merged

Fixes Parser.throwStatement() #510

merged 2 commits into from Jan 22, 2019

Conversation

stijnkliemesch
Copy link
Contributor

Fixes Parser.throwStatement() creating ThrowStatement with incorect length

Fixes Parser.throwStatement() creating ThrowStatement with incorect length
@stijnkliemesch
Copy link
Contributor Author

The created ThrowStatement had its length property initialized with not a length value but the ASTNode's endposition value.

@gbrail
Copy link
Collaborator

gbrail commented Jan 16, 2019

Very straightforward and it doesn't break anything. However in the service of making this codebase better over time I'd still love to see a new test case. Thanks!

This added testcase specifically fails without the fix from commit 52f97f8.

Without the fix: In TokenStream.getLine(int, int) the first assertion fails since the throw statement's end position exceeds the cursor (the whole script length even). This results in a thrown AssertionError (if the assert keyword is active in the java runtime).
@stijnkliemesch
Copy link
Contributor Author

The testcase triggers a specific use of the ThrowStatement instance that, without the fix, ends up using the incorrect value.

This case is how I encountered the bug, I suppose the bug mentioned in the testcase commit message should have been a seperately reported issue before initiating this pull request.

@gbrail
Copy link
Collaborator

gbrail commented Jan 22, 2019

Thanks for the test case!

@gbrail gbrail merged commit f875a68 into mozilla:master Jan 22, 2019
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.

None yet

2 participants