Added cursor to parser #287
Conversation
| } | ||
|
|
||
| macro_rules! expression { ( $name:ident, $lower:ident, [ $( $op:path ),* ] ) => { | ||
| fn $name (&mut self) -> ParseResult { | ||
| let mut lhs = self. $lower ()?; | ||
| while let Ok(tok) = self.peek_skip_lineterminator() { | ||
| while let Some(tok) = self.peek_skip_lineterminator().cloned() { |
There was a problem hiding this comment.
Could we somehow avoid this? It seems that Rust gives a "multiple mutable reference" error when you return a read-only reference from a mutable reference more than once. Is this something the compiler should understand it's safe?
There was a problem hiding this comment.
Hmm without checking this out and playing with it I’m not sure. Will need to take a look.
| let args = self.read_arguments()?; | ||
| let call_node = Node::Call(Box::new(lhs), args); | ||
|
|
||
| Node::New(Box::new(call_node)) | ||
| } else { | ||
| self.read_primary_expression()? | ||
| }; | ||
| while let Ok(tok) = self.peek_skip_lineterminator() { | ||
| match tok.kind { | ||
| while let Some(tok) = self.peek_skip_lineterminator().cloned() { |
There was a problem hiding this comment.
Could we somehow avoid this? It seems that Rust gives a "multiple mutable reference" error when you return a read-only reference from a mutable reference more than once. Is this something the compiler should understand it's safe?
| } | ||
|
|
||
| while let Ok(tok) = self.peek_skip_lineterminator() { | ||
| while let Some(tok) = self.peek_skip_lineterminator().cloned() { |
There was a problem hiding this comment.
Could we somehow avoid this? It seems that Rust gives a "multiple mutable reference" error when you return a read-only reference from a mutable reference more than once. Is this something the compiler should understand it's safe?
There was a problem hiding this comment.
Could we somehow avoid this? It seems that Rust gives a "multiple mutable reference" error when you return a read-only reference from a mutable reference more than once. Is this something the compiler should understand it's safe?
I found a way. If you take a &[Token] instead of an owning Vec<Token> in Cursor and you annotate the returning references with 'a, so &'a Token in functions that return it like next, next_if, etc. (also in Parser) you can eliminate the clone.
BTW I'm still new to rust lifetimes so there are probably better ways of doing this.
There was a problem hiding this comment.
Yeah I think this is correct, its the same thing I would have done.
|
It seems that this new implementation is giving a test error: |
| /// // Do some stuff that might change the cursor position... | ||
| /// cursor.seek(pos_save); | ||
| /// ``` | ||
| pub(super) fn seek(&mut self, pos: usize) { |
There was a problem hiding this comment.
Maybe it would make sense to abstract over this and provide a stack with breakpoints. So that we just say cursor.create_breakpoint() and then cursor.to_last_breakpoint(). But we need to remember to go back in all cases, or to remove the breakpoint if we won't use it anymore.
There was a problem hiding this comment.
So its worth pointing out that in future i would like to have the lexer send the parser tokens through a channel (most likely Crossbeam), this will allow us to have concurrent parsing (or streaming parsing as V8/Spidermonkey call it). It offers a big performance boost.
So you need to imagine we may not even have all the tokens yet, in which case seek may fail. (we jump too far ahead past the current-buffer)
I don't think we need to change things around now, but its worth building with that feature in mind.
I might just make an issue to discuss streaming parsing. Then we can tackle it after
There was a problem hiding this comment.
Moved into an issue here:
jasonwilliams#288
There was a problem hiding this comment.
Maybe it would make sense to abstract over this and provide a stack with breakpoints. So that we just say
cursor.create_breakpoint()and thencursor.to_last_breakpoint(). But we need to remember to go back in all cases, or to remove the breakpoint if we won't use it anymore.
I think seek() is fine for now.
Like you said with the breakpoint idea, it could open a huge rabbit hole
Co-Authored-By: HalidOdat <[email protected]>
|
Thanks @HalidOdat! This fixes the test, and I think this is ready for a merge :) |
This adds a simple cursor structure to the new parser (#281), which enables doing simpler operations, and making it easier to understand.
This also brings token referencing to the parser, to avoid so much cloning. Most of the cloning only happens when we need to return an error, in which case, it doesn't matter if it takes us a bit more. But for the rest, I think this should be faster. This closes #284.
I also took the opportunity to improve a bit further the documentation and the parser errors.