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

Split off parts of the runtime module into input, forest and parser. #115

Merged
merged 5 commits into from
May 15, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented May 14, 2019

Only runtime remains GLL-specific, the other modules can be moved to grammer at some point.

Copy link
Contributor

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

This definitely helps clean the code organization up!

I'm actually kind of eager to see the not-gll-specific parts pulled into grammer because that makes it easier for me to go and provide potential improvement refactoring 🙃

Let's wait for qmx or Centril to 👍 this though.

@@ -0,0 +1,546 @@
use crate::high::{type_lambda, ExistsL, PairL};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could generally use some doc comments on all pub items in the module.

if splits.len() > 1 {
return Err(MoreThanOne);
}
let &split = splits.iter().next().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have the same structure as in one_choice -- refactor out this bit?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're in different(ly typed) maps.

Copy link
Contributor

Choose a reason for hiding this comment

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

But there must be some common generic interface? Would be sad if not...

.get(&node)
.into_iter()
.flatten()
.cloned()
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything until here is also repeated in all_choices -- refactor out?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's... because this is how you iterate an Option<&Set<T>>?

.cloned()
.collect();
let mut seen: BTreeSet<_> = queue.iter().cloned().collect();
let mut p = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do better than just a one letter name p. :)

.collect();
let mut seen: BTreeSet<_> = queue.iter().cloned().collect();
let mut p = 0;
while let Some(source) = queue.pop_front() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to describe the algorithm used here so future readers need to think less.

None
}
match self.parser.input_consume_left(pat) {
Some(parser) => Some(Runtime {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some(parser) => Some(Runtime {
Some(parser) => Some(Self {

current: self.current,
saved: self.saved,
}),
None => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ?

})
}
match self.parser.input_consume_right(pat) {
Some(parser) => Some(Runtime {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some(parser) => Some(Runtime {
Some(parser) => Some(Self {

state: self.state,
current: self.current,
saved: self.saved,
}),
None => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also use ?

}
})
}
fn step<'i>(self, rt: Runtime<'_, 'i, Self, I>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn step<'i>(self, rt: Runtime<'_, 'i, Self, I>);
fn step(self, rt: Runtime<'_, '_, Self, I>);

@eddyb eddyb merged commit ae001ad into rust-lang:master May 15, 2019
@eddyb eddyb deleted the stratify branch May 15, 2019 07:46
@eddyb
Copy link
Member Author

eddyb commented May 15, 2019

I haven't addressed any of @Centril's comments (since basically everything is pre-existing, I just moved it around), but if @qmx, @CAD97, or anyone else wants to take a stab at them, I'd be glad to mentor!

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