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

Don't add comments to AST #393

Closed
wants to merge 1 commit into from

Conversation

gromgit
Copy link
Contributor

@gromgit gromgit commented Aug 1, 2020

It just confuses the REPL into printing nothing, even when it should.

Closes #388.

Copy link
Collaborator

@odino odino left a comment

Choose a reason for hiding this comment

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

Nice idea! I want to run a couple tests but this looks good at first glance

@gromgit
Copy link
Contributor Author

gromgit commented Aug 2, 2020

Forgot to explain the rationale for this change: Adding comments to the AST actually adds a final null object to the list of expressions that are evaluated. This means that the result of every commented statement is null, and while the effect in the REPL is mostly harmless (failure to print statement results), it will create logic bugs in any other context that tries to use the statement's results for further evaluation.

@odino
Copy link
Collaborator

odino commented Aug 5, 2020

BTW do you think we could do this in the lexer itself? So that we can ignore comments and there would be no reasons to even bring them down to the parser.

I think one of the downsides is that you might end up losing the commented blocks in error lines, but it might be that we currently don't support that either because of:

// We don't really have to do anything when comments
// come in, we can simply ignore them
func (p *Parser) parseComment() ast.Expression {
	return nil
}

@gromgit
Copy link
Contributor Author

gromgit commented Aug 5, 2020

BTW do you think we could do this in the lexer itself? So that we can ignore comments and there would be no reasons to even bring them down to the parser.

Yeah, that might actually work even better. Let me see how best to do it.

I think one of the downsides is that you might end up losing the commented blocks in error lines

I dunno about you, but when I see:

ERROR: identifier not found: nil
	[2:7]	 1 + nil # This should return 1

the trailing comment is just noise.

@odino
Copy link
Collaborator

odino commented Aug 5, 2020 via email

gromgit added a commit to gromgit/abs that referenced this pull request Aug 5, 2020
Alternate solution to abs-lang#388 (previous solution at abs-lang#393). This one skips over comments entirely, so no comment tokens are passed to callers.
@gromgit
Copy link
Contributor Author

gromgit commented Aug 5, 2020

Just submitted #394 that eliminates comments at the lexer level. These two PRs are mutually incompatible, so you can choose whichever you prefer, and close the other.

gromgit added a commit to gromgit/abs that referenced this pull request Aug 5, 2020
Alternate solution to abs-lang#388 (previous solution at abs-lang#393). This one skips over comments entirely, so no comment tokens are passed to callers.
gromgit added a commit to gromgit/abs that referenced this pull request Aug 6, 2020
Alternate solution to abs-lang#388 (previous solution at abs-lang#393). This one skips over comments entirely, so no comment tokens are passed to callers.
@gromgit
Copy link
Contributor Author

gromgit commented Aug 6, 2020

Since #394 seems to be the preferred approach, I'm closing this PR.

@gromgit gromgit closed this Aug 6, 2020
odino pushed a commit that referenced this pull request Aug 14, 2020
Alternate solution to #388 (previous solution at #393). This one skips over comments entirely, so no comment tokens are passed to callers.
odino pushed a commit that referenced this pull request Aug 14, 2020
Alternate solution to #388 (previous solution at #393). This one skips over comments entirely, so no comment tokens are passed to callers.
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.

REPL suppresses output in the presence of comments
2 participants