Refactor Parser.Stream#124
Open
AmosNico wants to merge 2 commits into
Open
Conversation
… instead of a variable stream
…sition, instead of a variable stream
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
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.
Currently all
Parser.Streaminstances (with the exception ofOfList, see later) use a wrapper type likeString.SliceorSubarrayas the stream. Internally the instances use the underlyingStringorArrayforParser.Stream.setPosition. This means that in practice there are two types of position used: the one on the level of the wrapper (i.eString.Posin the case ofString.Slice) and the one ofParser.Stream(i.e.String.Pos.Rawin the case ofString.Slice). The first one is used to update the position in the stream when reading forward, and the second one for storing and restoring the position.It would make more sense to use the same type for both purposes: Instead of changing the stream when moving forward, use a position to point to the current location in the stream, and update this instead when moving forward.
This pr implements this change, and also provides a new instance for
Parser.Stream String Char(which was not possible to do efficiently in the previous approach). The same could also be done forArrayandByteArray. I did not include them in the pr yet, as I am not sure whether they are needed: one can also just useSubArrayandByteSlice(the same remark holds forString).I removed
OfListcompletely, and replaced it by an instance forList. I think that storing a reference to a List (which is already in memory anyway) and using that list later on, is more efficient then the current approach where the list first needs to be reconstructed before it can be used again.Note that
Parser.Streamdoes not depend onStd.Streamany more. Instead it has its own version ofnext?, which uses positions instead of the stream itself.I also tried some variants:
String/String.Sliceparsers, it would be nice to useString.Pos/String.Slice.Posinstead of the raw versions (which would solve the proof issues in Replace Substring.Raw with String.Slice #123). This requires the position type to depend on the specific stream, which was previously not really feasible (since it changed constantly). Given that the parsed stream now remains unchanged, I tried adding it to the parser signature (i.e. addsto the signature, wheres : σ). This worked well, but it might be confusing for users, and I am not sure what the impact at runtime would be. Let me know if you would be interested in this approach, then I could tidy up the code and make a pull request.Parser.Streamdoes not depend onStd.Streamany more), but this caused issues inParser.Char(since this file works for all streams where the token type isChar).