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

Added checking in Error function in Parser #394

Closed
wants to merge 1 commit into from
Closed

Added checking in Error function in Parser #394

wants to merge 1 commit into from

Conversation

quinnreed
Copy link

In cases of TokenStream not being defined. Like in issue#386 #386

@gbrail
Copy link
Collaborator

gbrail commented Feb 27, 2018

Thanks!

I'd love to see a test!

I know that we don't have that many tests for the parser, but it seems that people are using Rhino to parse and help interpret JavaScript in lots of different projects and improving the parsers's quality via better code coverage would be a big help.

@@ -438,7 +438,6 @@ private static int YearFromTime(double t)

double y = Math.floor(t / (msPerDay * 365.2425)) + 1970;
double t2 = TimeFromYear(y);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is irrelevant and should be removed.

@@ -233,8 +233,12 @@ void addError(String messageId, int position, int length) {
}

void addError(String messageId, String messageArg) {
addError(messageId, messageArg, ts.tokenBeg,
ts.tokenEnd - ts.tokenBeg);
int beg = -1, end = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like us to adopt a single-assignment style for local variables in Rhino whenever possible, so

final int beg, end;
if (ts != null) {
    ...
} else {
    beg = end = -1;
}

would be much preferred.

@p-bakker
Copy link
Collaborator

The issue this PR fixes is already covered by #987, so closing this PR

@p-bakker p-bakker closed this Nov 29, 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.

4 participants