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

JavaScript parse tree visitor: check if ctx.children is non-null in visitChildren #2196

Merged
merged 2 commits into from
Jul 24, 2018
Merged

JavaScript parse tree visitor: check if ctx.children is non-null in visitChildren #2196

merged 2 commits into from
Jul 24, 2018

Conversation

robertbrignull
Copy link
Contributor

I've been encountering a problem where the default generated visitor in javascript. It will crash because in the visit method it tries to call accept on null because it has been passed a null object by visitChildren.

This seems like a bug to me as the children field is expected to sometimes be null as it is initialised to null and only made non-null when a child is added. Other places handle this case explicitly but here it's assumed to be non-null.

Making the change in this PR makes my visitor work correctly. Do you think it is the right change to make, or if not what should I be doing instead?


To explain my reasoning on why this change and not any other:

  • The check could easily be ctx.getChildCount() > 0 and behave the same. I went for this as it's shorter and we're already accessing ctx.children directly instead of using getChild so it seems in keeping with the current code.
  • The check could also have been done in visit by changing the else to else if (ctx) but this seemed a more intrusive way of fixing it and it's entirely reasonable for visit to expect a non-null argument.

return this.visit(ctx.children);
if (ctx.children) {
return this.visit(ctx.children);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add: else return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do

@ericvergnaud
Copy link
Contributor

@parrt blessed

@parrt parrt added this to the 4.7.2 milestone Jul 24, 2018
@parrt parrt merged commit fc01366 into antlr:master Jul 24, 2018
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.

3 participants