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.

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

Introduce the bruteforce parser (interpreter?). #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jul 23, 2019

Check out src/lyg.rs for an example involving pattern-matching parse nodes, and the two tests (ported over from the gll crate) for how to use the interpret and what the output is like.

I'm open to suggestions on the API and formatting, it's a bit arbitrary at the moment.

CAD97
CAD97 previously approved these changes Jul 23, 2019
Copy link
Collaborator

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

Everything looks fine. Obviously src/slow_bruteforce_interpreter.rs is the tricky part.

src/lyg.rs Outdated Show resolved Hide resolved
@eddyb
Copy link
Member Author

eddyb commented Jul 23, 2019

I think I'd like this to bake a while longer, at least to get to test it with wg-grammar and maybe try out some imperative disambiguation? Could result in a wildly different handle API.

@oli-obk also expressed interest in experimenting with the "web frontend" idea.

EDIT: some initial testing with wg-grammar results in a larger slowdown than expected, will investigate.
EDIT2: oops, that was a limitation in cyclotron, got 1000x speedup (or more) on some files.

Copy link
Collaborator

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

Generally: Everything outside slow_bruteforce_interpreter.rs looks good to me so far. I think some of the FIXME in that file can be pretty easily addressed, though. (Generally, it looks fine, but I've only really scanned that file so far.)

Copy link
Collaborator

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

Everything looks good here

src/slow_bruteforce_interpreter.rs Outdated Show resolved Hide resolved
@eddyb eddyb force-pushed the interpret branch 2 times, most recently from 1d4f1f5 to 2a3bfe1 Compare October 10, 2019 16:48
@eddyb eddyb changed the title Introduce the slow_bruteforce_interpreter. Introduce the bruteforce parser (interpreter?). Oct 10, 2019
};
}

testcases![
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to sync these with gll, additions were made there.

Rule::RepeatMore(rule, None) => {
NodeShape::Split(rule, cx.intern(Rule::RepeatMany(rule, None)))
Rule::RepeatMany(..) | Rule::RepeatMore(..) => {
NodeShape::Alias(self.expand_repeats(cx))
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be reverted/replaced with self.expand_repeats(cx).node_shape(cx, named_rules).

src/forest.rs Outdated
@@ -304,6 +309,93 @@ impl<'i, G: GrammarReflector, I: Input> ParseForest<'i, G, I> {
}
}

// TODO(eddyb) remove this entirely, only user of it left is `ListHandle`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be dealt with ASAP.

}
}
}
}
Copy link
Member Author

@eddyb eddyb Oct 10, 2019

Choose a reason for hiding this comment

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

So this is the nastiest part of parsing atm, an emulated stack to avoid blowing up the real one.

(Using a queue instead of a stack looks too much like GLL for my own comfort, otherwise I would've probably done it that way)

I'm wondering how I can make this more readable - maybe keep more things explicitly on the emulated stack? Use an enum with variants as an explicit state machine?

@eddyb eddyb force-pushed the interpret branch 3 times, most recently from 9f32647 to baec49d Compare October 19, 2019 13:28
Comment on lines +45 to +49
#[derive(Debug)]
enum SmallSet<T> {
One(T),
Many(BTreeSet<T>),
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Might be cheaper to use e.g. im::OrdSet for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants