Fix a few semicolon issues #54
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukaszsamson
commented
Feb 7, 2025
| @@ -225,6 +225,8 @@ defmodule Spitfire do | |||
| precedence < power | |||
| end | |||
|
|
|||
| # TODO Is anything needed here? Removing all of these tokens does not break any tests | |||
| # TODO Should we add :";" to the list? | |||
| @terminals MapSet.new([:eol, :eof, :"}", :")", :"]", :">>"]) | |||
There was a problem hiding this comment.
All tests pass even with
@terminals MapSet.new([])
lukaszsamson
commented
Feb 7, 2025
lib/spitfire.ex
Outdated
| @@ -306,6 +308,8 @@ defmodule Spitfire do | |||
| {parser, is_valid} = validate_peek(parser, current_token_type(parser)) | |||
|
|
|||
| if is_valid do | |||
| # TODO should we handle ; here? | |||
| # TODO removing peek_token(parser) != :eol does not break any tests | |||
| while is_nil(Map.get(parser, :stab_state)) and not MapSet.member?(terminals, peek_token(parser)) && | |||
There was a problem hiding this comment.
All tests pass with
(current_token(parser) != :do) &&
lukaszsamson
commented
Feb 7, 2025
lib/spitfire.ex
Outdated
| @@ -1542,6 +1546,8 @@ defmodule Spitfire do | |||
| else | |||
| {left, parser} = prefix.(parser) | |||
|
|
|||
| # TODO should we add ; here? | |||
| # TODO is anything needed? Removing all tokens does not break any tests | |||
| terminals = [:eol, :eof, :"}", :")", :"]", :">>"] | |||
There was a problem hiding this comment.
All tests pass with
terminals = []
lukaszsamson
commented
Feb 7, 2025
| @@ -2141,8 +2147,12 @@ defmodule Spitfire do | |||
| parser | |||
| end | |||
|
|
|||
| # TODO this may be too greedy. Probably the better option would be to have distinct eat_eol/1 and eat_eol_or_semicolon/1 | |||
| defp eat_eol(parser) do | |||
There was a problem hiding this comment.
eat_eol should be more context aware. Semicolons are not allowed in all contexts or change the meaning
lukaszsamson
commented
Feb 7, 2025
| @@ -2212,7 +2222,7 @@ defmodule Spitfire do | |||
| :eof | |||
| end | |||
|
|
|||
| defp peek_token_eat_eol(%{peek_token: {:eol, _token}} = parser) do | |||
There was a problem hiding this comment.
eat_eol should be more context aware. Semicolons are not allowed in all contexts or change the meaning
lukaszsamson
force-pushed
the
ls-fix-semicoln
branch
from
bb38d00 to
17d4b8f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes crash 1 from #48
I tried to find all the places where
:eoltoken was handled and by disabling that code discover tests that were affected. In a few places removing the code did not make any noticeable change so I left TODOs