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

Breaking tokens into pieces should behave similar to Parser::bump. #31455

Merged
merged 1 commit into from Feb 10, 2016
Merged

Breaking tokens into pieces should behave similar to Parser::bump. #31455

merged 1 commit into from Feb 10, 2016

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2016

Previously when breaking tokens into smaller pieces, the replace_token
function have been used. It replaced current token and updated span
information, but it did not clear the list of expected tokens, neither
did it update remaining info about last token. This could lead to
incorrect error message, like one described in the issue #24780:

expected one of ... `>` ...  found `>`

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks! Can you add a test case as well?

// for tokens currently using `bump_with`, last_token will be of no
// use anyway.
self.last_token = None;
self.last_token_interpolated = self.token.is_interpolated();
Copy link
Member

Choose a reason for hiding this comment

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

If you’re setting last_token to None, it makes little sense to store whether current token is Interpolated, right? This probably should be false unconditionally.

@ghost
Copy link
Author

ghost commented Feb 7, 2016

I added test based on #24780 and changed last_token_interpolated to be false
unconditionally.

Initially I was reluctant to add a test for this, because it seems to be
potentially fragile to changes in a parser (i.e., order of expected tokens), so
let me know if you have an idea for a better test.

// expected one of ..., `>`, ... found `>`
//
// compile-flags: -Z parse-only
// error-pattern: expected one of `!`, `::`, `where`, or `{`, found `>`
Copy link
Member

Choose a reason for hiding this comment

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

We typically use //~ ERROR instead of error-pattern as it matches exhaustively and also allows checking span (line number) information

Previously when breaking tokens into smaller pieces, the replace_token
function have been used. It replaced current token and updated span
information, but it did not clear the list of expected tokens, neither
did it update remaining info about last token. This could lead to
incorrect error message, like one described in the issue #24780:

  expected one of ... `>` ...  found `>`
@ghost
Copy link
Author

ghost commented Feb 8, 2016

Now using //~^ ERROR instead of error-pattern.

@alexcrichton
Copy link
Member

@bors: r+ cecf83f

@bors
Copy link
Collaborator

bors commented Feb 10, 2016

⌛ Testing commit cecf83f with merge 5d771cd...

bors added a commit that referenced this pull request Feb 10, 2016
Previously when breaking tokens into smaller pieces, the replace_token
function have been used. It replaced current token and updated span
information, but it did not clear the list of expected tokens, neither
did it update remaining info about last token. This could lead to
incorrect error message, like one described in the issue #24780:

    expected one of ... `>` ...  found `>`
@bors bors merged commit cecf83f into rust-lang:master Feb 10, 2016
@ghost ghost deleted the expected-tokens branch September 26, 2016 19:26
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.

6 participants