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

PEEK[start..end] #334

Merged
merged 27 commits into from
Nov 19, 2018
Merged

PEEK[start..end] #334

merged 27 commits into from
Nov 19, 2018

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Nov 6, 2018

fixes #329

Rule::quote,
Rule::single_quote
],
positives: vec![Rule::term],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no idea why I had to change this. is this a bug on master or did my added rule really change something?

Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a bug. I can take a closer look at it. It's weird that this particular example shouldn't be influenced by the changes you've made.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a jab at this right now to see exactly what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've fixed it in #343.

pest/src/parser_state.rs Outdated Show resolved Hide resolved
pest/src/parser_state.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dragostis dragostis left a comment

Choose a reason for hiding this comment

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

Very good progress! 👍

derive/src/lib.rs Show resolved Hide resolved
meta/src/grammar.pest Outdated Show resolved Hide resolved
Rule::quote,
Rule::single_quote
],
positives: vec![Rule::term],
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a bug. I can take a closer look at it. It's weird that this particular example shouldn't be influenced by the changes you've made.

pest/src/stack.rs Outdated Show resolved Hide resolved
pest/src/stack.rs Show resolved Hide resolved
meta/src/grammar.pest Show resolved Hide resolved
meta/src/parser.rs Outdated Show resolved Hide resolved
pest/src/parser_state.rs Outdated Show resolved Hide resolved
pest/src/stack.rs Show resolved Hide resolved
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Nov 9, 2018

OK, one problem:

   Compiling pest_grammars v2.0.0 (/home/phil/Dev/Rust/pest/grammars)
error[E0433]: failed to resolve. Use of undeclared type or module `state`
  --> derive/tests/grammar.rs:15:10
   |
15 | #[derive(Parser)]
   |          ^^^^^^ Use of undeclared type or module `state`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0433`.
error: Could not compile `pest_derive`.

I don’t know what I did differently than the other generator code. What did I miss?

generator/src/generator.rs Outdated Show resolved Hide resolved
generator/src/generator.rs Outdated Show resolved Hide resolved
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Nov 9, 2018

oh, whoops! thank you! I also fixed the next problem:

   Compiling pest_derive v2.0.1 (/home/phil/Dev/Rust/pest/derive)
error[E0308]: mismatched types
  --> derive/tests/grammar.rs:15:10
   |
15 | #[derive(Parser)]
   |          ^^^^^^
   |          |
   |          expected enum `std::option::Option`, found i32
   |          help: try using a variant of the expected type: `Some(-2i32)`
   |
   = note: expected type `std::option::Option<i32>`
              found type `i32`

the problem was that (Option as ToToken)::to_token only emits something if not empty!

I fixed it as described in dtolnay/quote#20

vm/tests/grammar.rs Outdated Show resolved Hide resolved
@flying-sheep flying-sheep changed the title PEEK[start..end] (work in progress) PEEK[start..end] Nov 9, 2018
Copy link
Contributor

@dragostis dragostis left a comment

Choose a reason for hiding this comment

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

This is pretty close. I'll take a look at the error reporting issue right now and let you know as soon as I figure it out.

derive/src/lib.rs Outdated Show resolved Hide resolved
meta/src/ast.rs Outdated Show resolved Hide resolved
meta/src/grammar.pest Outdated Show resolved Hide resolved
Rule::quote,
Rule::single_quote
],
positives: vec![Rule::term],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a jab at this right now to see exactly what's going on.

pub fn stack_match_peek_slice(mut self: Box<Self>, start: i32, end: Option<i32>, match_dir: MatchDir) -> ParseResult<Box<Self>> {
let range = match constrain_idxs(start, end, self.stack.len()) {
Some(r) => r,
None => return Err(self),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should being out-of-range be considered a hard error or a match fail? Currently, POP-ing on an empty stack causes a panic, but I feel like this approach is probably more expressive, if more error-prone.

pest/src/parser_state.rs Outdated Show resolved Hide resolved
pest/src/stack.rs Outdated Show resolved Hide resolved
pest/src/stack.rs Outdated Show resolved Hide resolved
@dragostis
Copy link
Contributor

Also, brackets should be added in pest_meta error renaming.

@flying-sheep flying-sheep force-pushed the peek-slice branch 2 times, most recently from 4307654 to ec331e9 Compare November 17, 2018 16:20
@flying-sheep
Copy link
Contributor Author

i added the brackets, all done!

Copy link
Contributor

@dragostis dragostis left a comment

Choose a reason for hiding this comment

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

LGTM apart from small fix!

vm/tests/grammar.rs Show resolved Hide resolved
@dragostis
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 19, 2018

👎 Rejected by code reviews

@dragostis
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 19, 2018

Merge conflict

@dragostis
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Nov 19, 2018
334: PEEK[start..end] r=dragostis a=flying-sheep

fixes #329

Co-authored-by: Philipp A <[email protected]>
Co-authored-by: Dragoș Tiselice <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 19, 2018

Build succeeded

@bors bors bot merged commit 7bf4ed6 into pest-parser:master Nov 19, 2018
@flying-sheep flying-sheep deleted the peek-slice branch November 19, 2018 11:22
@flying-sheep
Copy link
Contributor Author

Yay 🥳

Too bad it wasn't a squash merge, that's some messy series of commits 😅

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.

Handling indentation
3 participants