keep with_prototype when switching contexts with set
#177
Conversation
src/parsing/parser.rs
Outdated
| @@ -613,7 +613,9 @@ impl ParseState { | |||
| // top most on the stack after all the contexts are pushed - this is also | |||
| // referred to as the "target" of the push by sublimehq - see | |||
| // https://forum.sublimetext.com/t/dev-build-3111/19240/17 for more info | |||
| let proto = if i == ctx_refs.len() - 1 { | |||
| let proto = if i == 0 && pat.with_prototype.is_none() { | |||
There was a problem hiding this comment.
Maybe add a comment here, and move the existing comment into the if?
Also, I wasn't sure what ST does with a set with multiple contexts, have you tested that?
There was a problem hiding this comment.
I have, it is the same behavior as with push - the with_prototype only applies to the last context (that will be at the top of the stack after the set/push operation).
There was a problem hiding this comment.
I meant when we keep the existing with_prototype. In that case, should the code not use i == ctx_refs.len() - 1 instead of i == 0?
There was a problem hiding this comment.
you're quite right, thanks @robinst - I've corrected it and added some extra test cases for this.
|
LGTM. We could put both of the cases in a single |
src/parsing/parser.rs
Outdated
| // if a with_prototype was specified, and multiple contexts were pushed, | ||
| // then the with_prototype applies only to the last context pushed, i.e. | ||
| // top most on the stack after all the contexts are pushed - this is also | ||
| // referred to as the "target" of the push by sublimehq - see | ||
| // https://forum.sublimetext.com/t/dev-build-3111/19240/17 for more info | ||
| pat.with_prototype.clone() | ||
| pat.with_prototype.clone().or(old_proto.clone()) |
There was a problem hiding this comment.
I'm not sure if this is actually a hot spot or not, but I hadn't realized that or would need a clone. Maybe use or_else here.
There was a problem hiding this comment.
I'm having trouble with that, maybe you could take a quick go at this please:
the trait `std::ops::FnOnce<()>` is not implemented for `std::option::Option<std::rc::Rc<std::cell::RefCell<parsing::syntax_definition::Context>>>`
There was a problem hiding this comment.
Oh it takes a closure not a value, that's why it's more efficient, it only clones the value when necessary. So it would look like .or_else(|| old_proto.clone())
This implements @robinst's fix from #166 (comment) with a minimal test case, to keep the
with_prototypecontext active when a context isset- only when apopoccurs at that layer should thewith_prototypebe removed.