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

Remove `Option` from `TokenStream` #65261

Merged
merged 3 commits into from Oct 15, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Oct 10, 2019

A code simplification.

r? @petrochenkov

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 10, 2019

@bors try @rust-timer queue

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2019

⌛️ Trying commit 2ab66c5 with merge 3104977...

bors added a commit that referenced this pull request Oct 10, 2019
Remove `Option` from `TokenStream`

A code simplification.

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2019

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

@mati865

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 10, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 10, 2019

Queued 3104977 with parent aa45e03, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 11, 2019

Finished benchmarking try commit 3104977, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 11, 2019

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

This comment has been minimized.

Copy link
Contributor

mati865 commented Oct 11, 2019

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

Results are available but it's mostly noise.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 11, 2019

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.

src/libsyntax/tokenstream.rs Outdated Show resolved Hide resolved
src/libsyntax/tokenstream.rs Outdated Show resolved Hide resolved
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 11, 2019

LGTM, just a couple of nits.

nnethercote added 3 commits Oct 9, 2019
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.
This avoids some unnecessary creation of empty token streams.
@nnethercote nnethercote force-pushed the nnethercote:rm-Option-from-TokenStream branch from 2ab66c5 to 18b48bf Oct 13, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 13, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 14, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 14, 2019

📌 Commit 18b48bf has been approved by petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Oct 14, 2019
…eam, r=petrochenkov

Remove `Option` from `TokenStream`

A code simplification.

r? @petrochenkov
bors added a commit that referenced this pull request Oct 14, 2019
Rollup of 4 pull requests

Successful merges:

 - #64987 (Compute the layout of uninhabited structs)
 - #65252 (expand: Simplify expansion of derives)
 - #65260 (Optimize `LexicalResolve::expansion`.)
 - #65261 (Remove `Option` from `TokenStream`)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Oct 15, 2019
…eam, r=petrochenkov

Remove `Option` from `TokenStream`

A code simplification.

r? @petrochenkov
bors added a commit that referenced this pull request Oct 15, 2019
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
4 checks passed
4 checks passed
pr Build #20191013.65 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:rm-Option-from-TokenStream branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.