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 Duration to libcore (and remove deprecated Duration::span) #30594

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
8 participants
@SimonSapin
Copy link
Contributor

SimonSapin commented Dec 28, 2015

The Duration::span removal will hit stable in 1.7 at the soonest, while 1.6 already has the deprecation warning.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 28, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@SimonSapin SimonSapin force-pushed the SimonSapin:core-duration branch 2 times, most recently from acdfd75 to cb09b5e Dec 28, 2015

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Dec 28, 2015

cc @rust-lang/libs

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 29, 2015

Travis failure: https://travis-ci.org/rust-lang/rust/builds/99159735#L6416

I'm tentatively on board with this, but since libcore is stable now, we'll have to make sure we'll never want to add functionality that libcore can't support.

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Dec 29, 2015

It seems like it would be nice to have Duration unstable in core but stable in std, but I suspect this isn't currently possible.

@SimonSapin SimonSapin force-pushed the SimonSapin:core-duration branch from cb09b5e to 601e52d Dec 29, 2015

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 29, 2015

Pushed a fix, make check-stage1-bench passes locally.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 29, 2015

I'm tentatively on board with this, but since libcore is stable now, we'll have to make sure we'll never want to add functionality that libcore can't support.

This (having different set of functionality in core vs std for the same type) is not specific to Duration, and I expect we’ll encounter the same problem for other types in the future. The answer is extension traits, possibly in the prelude.

For str and char we’ve done this "backwards": libcore has StrExt and CharExt traits in its prelude with a subset of the functionality, while inherent method definitions are in libcollections and librustc_unicode. But this coherence-rules-bending trick only works for primitive types, right?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 29, 2015

I think Travis is stuck in a loop. I pushed hours ago and it’s now showing "Elapsed time 11 min 19 sec", but it was 20 minutes or so earlier.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 30, 2015

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

SimonSapin added some commits Dec 28, 2015

Remove the deprecated Duration::span
… so that Duration can be moved into libcore.

The removal will hit stable in 1.7 at the soonest,
while 1.6 already has the deprecation warning.

@SimonSapin SimonSapin force-pushed the SimonSapin:core-duration branch from 601e52d to ac249c1 Dec 30, 2015

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Dec 31, 2015

@SimonSapin

But this coherence-rules-bending trick only works for primitive types, right?

It's applicable to whatever types we like, but not something anyone has much of an appetite for doing. We did it for primitive types primarily because you hit them so early when learning Rust that extension traits seemed really problematic.

I agree with @huonw that ideally we'd have it unstable in core and stable in std, so that we can take our time on the stabilization aspect. @SimonSapin, would you mind trying that?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Jan 1, 2016

Re stability: sounds reasonable. I’ve pushed a commit that I think does this.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 6, 2016

@SimonSapin Can you add a test for stability in compile-fail?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 11, 2016

I think that lots of stability portions in the compiler don't expect a stable type with an unstable parent so it may not work out in terms of ensuring all usage through libcore is gated. For example all the method usage will be stable in libcore, it's just the module itself that's unstable. (which I forget how it affects the stability checking of a path like core::time::Duration)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2016

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

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 19, 2016

I'm going to flag for libs team discussion.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 4, 2016

The libs team discussed this PR during triage today, and the conclusion was to close this for now. Currently we think that moving a type from std to libcore needs motivation aside from "it can be done", as there are stabilization ramifications to moving into libcore rather than sticking with std (e.g. it's much harder to add functionality that requires std).

It was also brought up that this may not be too useful as the only member of core::time, which is where a motivating example would be more convincing here. Thanks for the PR though @SimonSapin!

@SimonSapin SimonSapin deleted the SimonSapin:core-duration branch Jun 26, 2018

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.