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

syntax: integrate TokenStream #40202

Merged
merged 6 commits into from
Mar 4, 2017
Merged

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Mar 2, 2017

Use TokenStream instead of Vec<TokenTree> in TokenTree::Delimited and elsewhere.
r? @nrc

@jseyfried
Copy link
Contributor Author

cc #38356
cc @abonander @keeperofdakeys

let ps = ParseSess::new();
filemap_to_tts(&ps, ps.codemap().new_filemap("bogofile".to_string(), None, source_str))
filemap_to_stream(&ps, ps.codemap().new_filemap("bogofile".to_string(), None, source_str))
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this filename appear anywhere? If so, it might be a good idea to use something more meaningful.

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 -- this is only used in internal unit tests.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

r=me with the renaming and comments

@@ -175,12 +175,107 @@ pub struct Parser<'a> {
/// into modules, and sub-parsers have new values for this name.
pub root_module_name: Option<String>,
pub expected_tokens: Vec<TokenType>,
pub tts: Vec<(TokenTree, usize)>,
cursor: Cursor,
Copy link
Member

Choose a reason for hiding this comment

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

nit: as a field name this is a bit uninformative, it would be good to have an indication of what this is a cursor over

pub desugar_doc_comments: bool,
/// Whether we should configure out of line modules as we parse.
pub cfg_mods: bool,
}

struct Cursor {
Copy link
Member

Choose a reason for hiding this comment

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

this could also have a more informative name I think - TokenCursor or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, TokenCursor sounds good.

@@ -396,6 +328,33 @@ impl Cursor {
}
}

#[derive(Debug, Clone)]
pub struct ThinTokenStream(Option<RcSlice<TokenStream>>);
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have a comment describing what this is and where it should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Mar 3, 2017

📌 Commit 0d55413 has been approved by nrc

@bors
Copy link
Contributor

bors commented Mar 3, 2017

⌛ Testing commit 0d55413 with merge bd77267...

@bors
Copy link
Contributor

bors commented Mar 3, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Mar 3, 2017 via email

@bors
Copy link
Contributor

bors commented Mar 4, 2017

⌛ Testing commit 0d55413 with merge be304af...

bors added a commit that referenced this pull request Mar 4, 2017
syntax: integrate `TokenStream`

Use `TokenStream` instead of `Vec<TokenTree>` in `TokenTree::Delimited` and elsewhere.
r? @nrc
@bors
Copy link
Contributor

bors commented Mar 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing be304af to master...

@bors bors merged commit 0d55413 into rust-lang:master Mar 4, 2017
@jseyfried jseyfried deleted the integrate_tokenstream branch March 8, 2017 05:25
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.

5 participants