Feedback on new support for more than one look-ahead tokens #8
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.
@atifaziz here is a quick PR on top of your
multi-peek
branch.My question is what is the purpose of the
default
case in thePeek()
method ?It seems to me that support for more than two look-ahead tokens is guaranteed to throw an exception.
I added a simple test illustrate this.
I like the
ITokenStream
abstraction that as been introduced.As a general question why limit the implementation to only two tokens?
I guess there should not be a grammar that needs more than two tokens of lookahead.
Also, it seems that maybe the design you chose is extensible to more than two tokens of lookahead in the future, maybe by introducing a future
ThreeTokenStackOps
, etc. But what lead you to this design, with two differentStoreStackOps
implementations? Is this for performance reason ? Is this in an attempt to prevent more upfront allocations for multiple tokens if not eventually necessary?