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 `tokenstream::Delimited`. #56369

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
6 participants
@nnethercote
Contributor

nnethercote commented Nov 30, 2018

Because it's an extra type layer that doesn't really help; in a couple
of places it actively gets in the way, and overall removing it makes the
code nicer. It does, however, move tokenstream::TokenTree further away
from the TokenTree in quote.rs.

More importantly, this change reduces the size of TokenStream from 48
bytes to 40 bytes on x86-64, which is enough to slightly reduce
instruction counts on numerous benchmarks, the best by 1.5%.

Note that open_tt and close_tt have gone from being methods on
Delimited to associated methods of TokenTree.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 30, 2018

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 30, 2018

Marking as blocked on #49219.

cc @dtolnay @alexcrichton

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 30, 2018

Why is it blocked on #49219?

@bors

This comment has been minimized.

Contributor

bors commented Nov 30, 2018

☔️ The latest upstream changes (presumably #49219) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 30, 2018

@nnethercote
You'll see it when you start rebasing :)

@petrochenkov petrochenkov self-assigned this Nov 30, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 30, 2018

#49219 has landed; @nnethercote should be able to rebase this now. :)

@nnethercote nnethercote force-pushed the nnethercote:rm-Delimited branch from 00e699a to 8e4ccef Dec 10, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 10, 2018

@eddyb: I have rebased.

@@ -146,15 +102,15 @@ impl TokenTree {
pub fn span(&self) -> Span {
match *self {
TokenTree::Token(sp, _) => sp,
TokenTree::Delimited(sp, _) => sp.entire(),
TokenTree::Delimited(sp, _, _) => sp.entire(),

This comment has been minimized.

@petrochenkov

petrochenkov Dec 10, 2018

Contributor

Nit: _, _ -> ..

This comment has been minimized.

@petrochenkov

petrochenkov Dec 10, 2018

Contributor

(There are many instances of this.)

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 10, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Dec 10, 2018

📌 Commit 8e4ccef has been approved by petrochenkov

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 10, 2018

In case you'll want to fix #56369 (comment)
@bors delegate+

@bors

This comment has been minimized.

Contributor

bors commented Dec 10, 2018

✌️ @nnethercote can now approve this pull request

Remove `tokenstream::Delimited`.
Because it's an extra type layer that doesn't really help; in a couple
of places it actively gets in the way, and overall removing it makes the
code nicer. It does, however, move `tokenstream::TokenTree` further away
from the `TokenTree` in `quote.rs`.

More importantly, this change reduces the size of `TokenStream` from 48
bytes to 40 bytes on x86-64, which is enough to slightly reduce
instruction counts on numerous benchmarks, the best by 1.5%.

Note that `open_tt` and `close_tt` have gone from being methods on
`Delimited` to associated methods of `TokenTree`.

@nnethercote nnethercote force-pushed the nnethercote:rm-Delimited branch from 8e4ccef to 1fe2c03 Dec 10, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 10, 2018

I addressed the nits.

@bors r=petrochenkov

@bors

This comment has been minimized.

Contributor

bors commented Dec 10, 2018

📌 Commit 1fe2c03 has been approved by petrochenkov

@bors

This comment has been minimized.

Contributor

bors commented Dec 10, 2018

⌛️ Testing commit 1fe2c03 with merge 286dc37...

bors added a commit that referenced this pull request Dec 10, 2018

Auto merge of #56369 - nnethercote:rm-Delimited, r=petrochenkov
Remove `tokenstream::Delimited`.

Because it's an extra type layer that doesn't really help; in a couple
of places it actively gets in the way, and overall removing it makes the
code nicer. It does, however, move `tokenstream::TokenTree` further away
from the `TokenTree` in `quote.rs`.

More importantly, this change reduces the size of `TokenStream` from 48
bytes to 40 bytes on x86-64, which is enough to slightly reduce
instruction counts on numerous benchmarks, the best by 1.5%.

Note that `open_tt` and `close_tt` have gone from being methods on
`Delimited` to associated methods of `TokenTree`.
@bors

This comment has been minimized.

Contributor

bors commented Dec 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 286dc37 to master...

@bors bors merged commit 1fe2c03 into rust-lang:master Dec 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:rm-Delimited branch Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment