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

Move gensym operations from `Symbol` to `Ident` #60903

Merged
merged 3 commits into from May 21, 2019

Conversation

Projects
None yet
5 participants
@nnethercote
Copy link
Contributor

commented May 17, 2019

Gensyms are always at the Ident level, and long-term we probably want to record gensym-ness in hygiene data.

r? @petrochenkov

@@ -613,10 +613,12 @@ pub struct Ident {

impl Ident {
#[inline]
/// constructs a new identifier from a symbol and a span.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov May 17, 2019

Contributor
Suggested change
/// constructs a new identifier from a symbol and a span.
/// Constructs a new identifier from a symbol and a span.
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

r=me after rebase

@nnethercote nnethercote force-pushed the nnethercote:mv-gensyms-from-Symbol-to-Ident branch from 17b5966 to ad5dd49 May 19, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

⌛️ Trying commit ad5dd49 with merge 139b5da...

bors added a commit that referenced this pull request May 19, 2019

Auto merge of #60903 - nnethercote:mv-gensyms-from-Symbol-to-Ident, r…
…=<try>

Move gensym operations from `Symbol` to `Ident`

Gensyms are always at the `Ident` level, and long-term we probably want to record gensym-ness in hygiene data.

r? @petrochenkov

@nnethercote nnethercote force-pushed the nnethercote:mv-gensyms-from-Symbol-to-Ident branch from ad5dd49 to 8a96f26 May 19, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

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

nnethercote added some commits May 16, 2019

Move `is_gensymed` from `Symbol` to `Ident`.
Note that the `is_gensymed` call on `primitive_types` is unnecessary
because that table only contains the name of primitive types (e.g.
`i32`) and never contains gensyms.

@nnethercote nnethercote force-pushed the nnethercote:mv-gensyms-from-Symbol-to-Ident branch from 8a96f26 to 88d2999 May 20, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

⌛️ Trying commit 88d2999 with merge e67ebe6...

bors added a commit that referenced this pull request May 20, 2019

Auto merge of #60903 - nnethercote:mv-gensyms-from-Symbol-to-Ident, r…
…=<try>

Move gensym operations from `Symbol` to `Ident`

Gensyms are always at the `Ident` level, and long-term we probably want to record gensym-ness in hygiene data.

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

☀️ Try build successful - checks-travis
Build commit: e67ebe6

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 20, 2019

Success: Queued e67ebe6 with parent 128b4c8, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 20, 2019

Finished benchmarking try commit e67ebe6: comparison url

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

It's a slight performance regression, but I think it's necessary to take at some point in order to move gensym-ness away from symbols. We've had a bunch of significantly larger performance improvements related to symbol handling recently, which more than make up for this.

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

📌 Commit 88d2999 has been approved by petrochenkov

bors added a commit that referenced this pull request May 21, 2019

Auto merge of #60903 - nnethercote:mv-gensyms-from-Symbol-to-Ident, r…
…=petrochenkov

Move gensym operations from `Symbol` to `Ident`

Gensyms are always at the `Ident` level, and long-term we probably want to record gensym-ness in hygiene data.

r? @petrochenkov
@bors

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

⌛️ Testing commit 88d2999 with merge 50a0def...

@bors

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 50a0def to master...

@bors bors added the merged-by-bors label May 21, 2019

@bors bors merged commit 88d2999 into rust-lang:master May 21, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:mv-gensyms-from-Symbol-to-Ident branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.