Skip to content

Remove Option from TokenStream #65261

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 15, 2019
Merged

Remove Option from TokenStream #65261

merged 3 commits into from
Oct 15, 2019

Conversation

nnethercote
Copy link
Contributor

A code simplification.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2019
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@bors
Copy link
Collaborator

bors commented Oct 10, 2019

⌛ Trying commit 2ab66c543bea042971bb502c19471ec6827c625e with merge 310497755aba58c78b873085b89172ca0a8b0cdc...

@bors
Copy link
Collaborator

bors commented Oct 10, 2019

☀️ Try build successful - checks-azure
Build commit: 310497755aba58c78b873085b89172ca0a8b0cdc (310497755aba58c78b873085b89172ca0a8b0cdc)

csmoe
csmoe reviewed Oct 10, 2019
View reviewed changes
@mati865
Copy link
Member

mati865 commented Oct 10, 2019

This perf seems to be missing from perf.rlo queue, maybe bot was down while you were scheduling.

@Mark-Simulacrum
Copy link
Member

@rust-timer build 310497755aba58c78b873085b89172ca0a8b0cdc

@rust-timer
Copy link
Collaborator

Queued 310497755aba58c78b873085b89172ca0a8b0cdc with parent aa45e03, future comparison URL.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2019
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 310497755aba58c78b873085b89172ca0a8b0cdc, comparison URL.

@nnethercote
Copy link
Contributor Author

Finished benchmarking try commit 3104977, comparison URL.

Two hours later, I get "could not find commit by bound Commit("aa45e032d96f1785581d336170e6dc35d5f1cb65"), start=true" when I visit the link. rust-timer is really struggling lately :(

cc @Mark-Simulacrum

@mati865
Copy link
Member

mati865 commented Oct 11, 2019

I guess timer bot doesn't check whether TryParent has finished.

Results are available but it's mostly noise.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 11, 2019
@nnethercote
Copy link
Contributor Author

Results are available but it's mostly noise.

Thanks for letting me know. Noise is good in this case; it shows that it doesn't hurt performance.

petrochenkov
petrochenkov reviewed Oct 11, 2019
View reviewed changes
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2019
@petrochenkov
Copy link
Contributor

LGTM, just a couple of nits.

@nnethercote
Remove the Option in TokenStream.
5c93492 
It means an allocation is required to create an empty `TokenStream`, but
all other operations are simpler and marginally faster due to not having
to check for `None`. Overall it simplifies the code for a negligible
performance effect.

The commit also removes `TokenStream::empty` by implementing `Default`,
which is now possible.
@nnethercote
Use TokenStream::default() in more places.
1ce0347
@nnethercote
Lazify some mac_placeholder() calls.
18b48bf 
This avoids some unnecessary creation of empty token streams.
@nnethercote nnethercote force-pushed the rm-Option-from-TokenStream branch from 2ab66c5 to 18b48bf Compare October 13, 2019 23:29
@nnethercote
Copy link
Contributor Author

@petrochenkov: New code is up, with nits addressed.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 14, 2019

📌 Commit 18b48bf has been approved by petrochenkov

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 14, 2019
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 14, 2019
@Centril
Rollup merge of rust-lang#65261 - nnethercote:rm-Option-from-TokenStr…
ed9d45b 
…eam, r=petrochenkov

Remove `Option` from `TokenStream`

A code simplification.

r? @petrochenkov
@Centril Centril mentioned this pull request Oct 14, 2019
Closed
tmandry added a commit to tmandry/rust that referenced this pull request Oct 15, 2019
@tmandry
Rollup merge of rust-lang#65261 - nnethercote:rm-Option-from-TokenStr…
6d28ed1 
…eam, r=petrochenkov

Remove `Option` from `TokenStream`

A code simplification.

r? @petrochenkov
@tmandry tmandry mentioned this pull request Oct 15, 2019
Merged
bors added a commit that referenced this pull request Oct 15, 2019
@bors
Auto merge of #65422 - tmandry:rollup-r5u3mlc, r=tmandry
e369d87 
Rollup of 10 pull requests

Successful merges:

 - #65170 (rustc_metadata: Privatize private code and remove dead code)
 - #65260 (Optimize `LexicalResolve::expansion`.)
 - #65261 (Remove `Option` from `TokenStream`)
 - #65332 (std::fmt: reorder docs)
 - #65340 (Several changes to the codegen backend organization)
 - #65365 (Include const generic arguments in metadata)
 - #65398 (Bring attention to suggestions when the only difference is capitalization)
 - #65410 (syntax: add parser recovery for intersection- / and-patterns `p1 @ p2`)
 - #65415 (Remove an outdated test output file)
 - #65416 (Minor sync changes)

Failed merges:

r? @ghost
@bors bors merged commit 18b48bf into rust-lang:master Oct 15, 2019
@nnethercote nnethercote deleted the rm-Option-from-TokenStream branch October 15, 2019 05:40
@nnethercote nnethercote mentioned this pull request Oct 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

+1 more reviewer

@csmoe csmoe csmoe left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nnethercote @bors @mati865 @Mark-Simulacrum @rust-timer @petrochenkov @csmoe @rust-highfive