Skip to content

Introduce the bruteforce parser (interpreter?).#10

Open
eddyb wants to merge 1 commit into
masterfrom
interpret
Open

Introduce the bruteforce parser (interpreter?).#10
eddyb wants to merge 1 commit into
masterfrom
interpret

Conversation

@eddyb

@eddyb eddyb commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

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

@CAD97 CAD97 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/lyg.rs Outdated
@eddyb

eddyb commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/slow_bruteforce_interpreter.rs Outdated

@CAD97 CAD97 left a comment

Copy link
Copy Markdown
Contributor

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

@CAD97 CAD97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything looks good here

Comment thread src/slow_bruteforce_interpreter.rs Outdated
@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
Comment thread tests/basic.rs
};
}

testcases![

Copy link
Copy Markdown
Contributor 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.

Comment thread src/rule.rs
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
Copy Markdown
Contributor 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).

Comment thread src/forest.rs Outdated
}
}

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

Copy link
Copy Markdown
Contributor 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.

Comment thread src/bruteforce.rs Outdated
}
}
}
}

@eddyb eddyb Oct 10, 2019

Copy link
Copy Markdown
Contributor Author

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 thread Cargo.toml
Comment thread src/bruteforce.rs
Comment on lines +45 to +49
#[derive(Debug)]
enum SmallSet<T> {
One(T),
Many(BTreeSet<T>),
}

Copy link
Copy Markdown
Contributor 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