Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keep with_prototype when switching contexts with set #177

Merged
merged 6 commits into from
Jun 12, 2018
Merged

keep with_prototype when switching contexts with set #177

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbf6417
keep with_prototype when switching contexts with `set`
keith-hall Jun 11, 2018
74eda91
add comment explaining `set` `with_prototype` behavior
keith-hall Jun 11, 2018
37533e1
correctly keep with_prototype on `set` for top context only
keith-hall Jun 11, 2018
250584f
test set multiple contexts when with_prototype already active
keith-hall Jun 11, 2018
6d963fb
simplify if statement
keith-hall Jun 11, 2018
c0e1a2c
use closure for efficiency
keith-hall Jun 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 75 additions & 10 deletions src/parsing/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,11 +595,11 @@ impl ParseState {

/// Returns true if the stack was changed
fn perform_op(&mut self, line: &str, regions: &Region, pat: &MatchPattern) -> bool {
let ctx_refs = match pat.operation {
MatchOperation::Push(ref ctx_refs) => ctx_refs,
let (ctx_refs, old_proto) = match pat.operation {
MatchOperation::Push(ref ctx_refs) => (ctx_refs, None),
MatchOperation::Set(ref ctx_refs) => {
self.stack.pop();
ctx_refs
let old_proto = self.stack.pop().and_then(|s| s.prototype);
(ctx_refs, old_proto)
}
MatchOperation::Pop => {
self.stack.pop();
Expand All @@ -608,13 +608,16 @@ impl ParseState {
MatchOperation::None => return false,
};
for (i, r) in ctx_refs.iter().enumerate() {
// 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
let proto = if i == ctx_refs.len() - 1 {
pat.with_prototype.clone()
// a `with_prototype` stays active when the context is `set`
// until the context layer in the stack (where the `with_prototype`
// was initially applied) is popped off.
// 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().or(old_proto.clone())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>>`

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

} else {
None
};
Expand Down Expand Up @@ -1257,6 +1260,68 @@ contexts:
expect_scope_stacks_with_syntax("/** * */", &["<comment.block.documentation.javadoc>, <punctuation.definition.comment.begin.javadoc>", "<comment.block.documentation.javadoc>, <text.html.javadoc>, <punctuation.definition.comment.javadoc>", "<comment.block.documentation.javadoc>, <punctuation.definition.comment.end.javadoc>"], syntax);
}

#[test]
fn can_parse_with_prototype_set() {
let syntax = r#"%YAML 1.2
---
scope: source.test-set-with-proto
contexts:
main:
- match: a
scope: a
set: next1
with_prototype:
- match: '1'
scope: '1'
- match: '2'
scope: '2'
- match: '3'
scope: '3'
- match: '4'
scope: '4'
- match: '5'
scope: '5'
set: [next3, next2]
with_prototype:
- match: c
scope: cwith
next1:
- match: b
scope: b
set: next2
next2:
- match: c
scope: c
push: next3
- match: e
scope: e
pop: true
- match: f
scope: f
set: [next1, next2]
next3:
- match: d
scope: d
- match: (?=e)
pop: true
- match: c
scope: cwithout
"#;

expect_scope_stacks_with_syntax(
"a1b2c3d4e5",
&[
"<a>", "<1>", "<b>", "<2>", "<c>", "<3>", "<d>", "<4>", "<e>", "<5>"
], SyntaxDefinition::load_from_str(&syntax, true, None).unwrap()
);
expect_scope_stacks_with_syntax(
"5cfcecbedcdea",
&[
"<5>", "<cwith>", "<f>", "<e>", "<b>", "<d>", "<cwithout>", "<a>"
], SyntaxDefinition::load_from_str(&syntax, true, None).unwrap()
);
}

fn expect_scope_stacks(line_without_newline: &str, expect: &[&str], syntax: &str) {
println!("Parsing with newlines");
let line_with_newline = format!("{}\n", line_without_newline);
Expand Down