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

specialize ToString for str #32586

Merged
merged 1 commit into from Apr 1, 2016

Conversation

Projects
None yet
9 participants
@seanmonstar
Copy link
Contributor

seanmonstar commented Mar 29, 2016

If there was some conditional compiling we could do, such that this impl only exists in nightly, and is turned off in beta/stable, I think that'd be an improvement here, as we could test specialization out without affecting stable builds.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 29, 2016

r? @alexcrichton

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

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 29, 2016

@sfackler sfackler added the T-libs label Mar 29, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 29, 2016

Thanks for the PR! @rust-lang/libs actually just started talking about whether we want to use specialization in the standard library. The conclusion so far is that we don't want want to make any strong commitments in terms of enabling new functionality just yet (as specialization isn't 100% on the path to stabilization yet), but small perf improvements like this seem reasonable for the standard library to do.

I'm personally in favor of a change like this to test out specialization, but if we hit ICEs here and there we should be ready to revert relatively quickly.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 30, 2016

Does #[unstable] have any effect on impl?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 30, 2016

Nope - it should probably be change to stable.

@seanmonstar seanmonstar force-pushed the seanmonstar:speialize-to-string branch from 67fac0c to fc8cf9c Mar 30, 2016

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 30, 2016

Fixed stability attribute.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 31, 2016

The libs team discussed this during triage today and the decision was to merge! This does not expand the standard library's API surface area, provides a clear performance benefit, and it's very difficult to rely on specialization where code would break if we removed it for whatever reason. @aturon was also "very confident" that this would not ICE :)

@bors: r+ fc8cf9c

Thanks for the PR @seanmonstar!

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Mar 31, 2016

@alexcrichton ... I did suggest a crater run ;-)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 31, 2016

Oh oops I may have misinterpreted in that case. I'll schedule a crater run.

@bors: r-

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 31, 2016

Crater says zero regressions, yay!

@bors: r+ fc8cf9c

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Apr 1, 2016

⌛️ Testing commit fc8cf9c with merge 3b342fa...

bors added a commit that referenced this pull request Apr 1, 2016

Auto merge of #32586 - seanmonstar:speialize-to-string, r=alexcrichton
specialize ToString for str

If there was some conditional compiling we could do, such that this impl only exists in nightly, and is turned off in beta/stable, I think that'd be an improvement here, as we could test specialization out without affecting stable builds.

@bors bors merged commit fc8cf9c into rust-lang:master Apr 1, 2016

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

bluss commented Apr 1, 2016

Cool! 💀 to the ".to_string()" is slow memes!

More procedural question: This type of change makes the blanket ToString impl downstream specializable. Do we want this? What do we need to think of when deciding whether to allow that with a default or not?

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Apr 1, 2016

@bluss Very good question. We talked about this some in a recent libs meeting -- ultimately, we're going to want a conventions RFC here, I think. But it's hard to get there without some experimentation first.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Apr 3, 2016

@bluss

This type of change makes the blanket ToString impl downstream specializable. Do we want this? What do we need to think of when deciding whether to allow that with a default or not?

RFC 1422 gives a general way to limit various permissions. The RFC talks only about permission to access (pub(path)), but the same mechanism can be also applied to permissions to specialize , permissions to implement and even permissions to mutate maybe.

In this particular case something like default(crate) could be used to limit the permission to specialize this method to the local crate until the decision to make it specializabe by third parties is achieved.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Apr 3, 2016

@petrochenkov You could take the approach in PR #32699 instead if specialization is not open to the outside (use an intermediate internal trait).

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