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 #46666

Merged
merged 1 commit into from Jan 31, 2018

Conversation

Projects
None yet
9 participants
@clarcharr
Contributor

clarcharr commented Dec 11, 2017

Fixes #46520; should be merged after #46508.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 11, 2017

Collaborator

r? @sfackler

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

Collaborator

rust-highfive commented Dec 11, 2017

r? @sfackler

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

Show outdated Hide outdated src/libcore/time.rs Outdated
Show outdated Hide outdated src/libcore/time.rs Outdated
@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 20, 2017

Member

#46508 has been merged. @clarcharr

Member

kennytm commented Dec 20, 2017

#46508 has been merged. @clarcharr

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

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

Contributor

bors commented Dec 20, 2017

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

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Dec 21, 2017

Contributor

@kennytm Just rebased! Waiting on CI.

Contributor

clarcharr commented Dec 21, 2017

@kennytm Just rebased! Waiting on CI.

Show outdated Hide outdated src/libcore/time.rs Outdated
Show outdated Hide outdated src/libcore/time.rs Outdated
@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Dec 21, 2017

Contributor

(it is passing now)

Contributor

clarcharr commented Dec 21, 2017

(it is passing now)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

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

Contributor

bors commented Dec 22, 2017

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

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler
Member

sfackler commented Dec 23, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 23, 2017

Contributor

📌 Commit 3f6b8e1 has been approved by sfackler

Contributor

bors commented Dec 23, 2017

📌 Commit 3f6b8e1 has been approved by sfackler

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler
Member

sfackler commented Dec 23, 2017

@bors r-

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Dec 23, 2017

Member

Er, actually, let's make sure the rest of the libs folks are okay with this since the move is insta-stable.

@rfcbot fcp merge

Member

sfackler commented Dec 23, 2017

Er, actually, let's make sure the rest of the libs folks are okay with this since the move is insta-stable.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 23, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Dec 23, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 26, 2017

Member

@rfcbot concern we-may-want-std

This was previously proposed in #30594 and we closed with this comment #30594 (comment). Has something changed in the meantime though?

Member

alexcrichton commented Dec 26, 2017

@rfcbot concern we-may-want-std

This was previously proposed in #30594 and we closed with this comment #30594 (comment). Has something changed in the meantime though?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Dec 26, 2017

Contributor

I think a good example is the fact that there are a lot of libcore-compatible libraries that need to track time in a system-independent way. After all, SystemTime and Instant fundamentally represent Durations, but they're opaque because they offer system-specific guarantees and drawbacks.

I can't really think of anything platform-dependent we'd want on Duration, considering how it's supposed to be the platform-independent representation of time. Any methods that would get a duration relative to something would likely be encapsulated in their own types.

Contributor

clarcharr commented Dec 26, 2017

I think a good example is the fact that there are a lot of libcore-compatible libraries that need to track time in a system-independent way. After all, SystemTime and Instant fundamentally represent Durations, but they're opaque because they offer system-specific guarantees and drawbacks.

I can't really think of anything platform-dependent we'd want on Duration, considering how it's supposed to be the platform-independent representation of time. Any methods that would get a duration relative to something would likely be encapsulated in their own types.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Dec 26, 2017

Contributor

Also just in general Duration is one of the types that's simple yet complex enough that the boilerplate required to fully recreate it is nontrivial in downstream crates. Implementing all of the numeric traits it offers would be a real pain, and coding methods for specific units of time has a lot of room for error.

It just makes the most sense to put this in libcore.

Contributor

clarcharr commented Dec 26, 2017

Also just in general Duration is one of the types that's simple yet complex enough that the boilerplate required to fully recreate it is nontrivial in downstream crates. Implementing all of the numeric traits it offers would be a real pain, and coding methods for specific units of time has a lot of room for error.

It just makes the most sense to put this in libcore.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 2, 2018

Member

Do you have a set of concrete compelling cases for this PR? To bring it up with the libs team I'd like to have a set of cases to talk about which we were specifically lacking last time. "it just makes the most sense" is why we rejected this the first time around, so to consider it again we just want to make sure there's new information to consider.

Member

alexcrichton commented Jan 2, 2018

Do you have a set of concrete compelling cases for this PR? To bring it up with the libs team I'd like to have a set of cases to talk about which we were specifically lacking last time. "it just makes the most sense" is why we rejected this the first time around, so to consider it again we just want to make sure there's new information to consider.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 10, 2018

Member

Hi @clarcharr! The libs team still has a concern for the motivation of this PR. Would you resolve it?

Member

kennytm commented Jan 10, 2018

Hi @clarcharr! The libs team still has a concern for the motivation of this PR. Would you resolve it?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jan 12, 2018

Contributor

@alexcrichton I mean, that PR is two years old and Rust has gained a lot of no_std crates in that timeframe. I don't really know what sort of examples I can give, other than stating that time measurement is something that shows up in embedded systems all the time. If I wanted to code a simple OS in Rust I'd have to do all time measurement with a custom Duration struct, or probably just an int measuring nanoseconds, which seems unnecessary considering how all that work is already done in the standard library.

Pre-1.0 I actually used a bit of Rust for a systems programming class, and although Duration didn't exist at the time, if I were to code such a thing now I'd definitely be a bit upset that this is included in libstd despite definitely being a system-agnostic type.

To flip the question: do you have any vague idea of what an std-requiring Duration would look like? What kind of functionality would need to be system-specific and not included in a separate type like SystemTime or Instant?

(to add: if Duration represented a moment in time I would absolutely agree with you, but it represents the distance between two points in time, which is much different)

Contributor

clarcharr commented Jan 12, 2018

@alexcrichton I mean, that PR is two years old and Rust has gained a lot of no_std crates in that timeframe. I don't really know what sort of examples I can give, other than stating that time measurement is something that shows up in embedded systems all the time. If I wanted to code a simple OS in Rust I'd have to do all time measurement with a custom Duration struct, or probably just an int measuring nanoseconds, which seems unnecessary considering how all that work is already done in the standard library.

Pre-1.0 I actually used a bit of Rust for a systems programming class, and although Duration didn't exist at the time, if I were to code such a thing now I'd definitely be a bit upset that this is included in libstd despite definitely being a system-agnostic type.

To flip the question: do you have any vague idea of what an std-requiring Duration would look like? What kind of functionality would need to be system-specific and not included in a separate type like SystemTime or Instant?

(to add: if Duration represented a moment in time I would absolutely agree with you, but it represents the distance between two points in time, which is much different)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 12, 2018

Contributor

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

Contributor

bors commented Jan 12, 2018

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 16, 2018

Member

Ah sorry I meant to comment on this but forgot! The @rust-lang/libs team discussed this PR at triage last week and the conclusion was that we should go ahead and move forward with it. Our thinking was that we can't think of any functionality we'd really like to add to Duration any time soon which requires libstd, and if this does come about we can always figure out a solution. In the meantime Duration also has a pro in that it's so simple, so it seems fine to include in libcore.

@clarcharr want to rebase before merging?

@rfcbot fcp merge

Member

alexcrichton commented Jan 16, 2018

Ah sorry I meant to comment on this but forgot! The @rust-lang/libs team discussed this PR at triage last week and the conclusion was that we should go ahead and move forward with it. Our thinking was that we can't think of any functionality we'd really like to add to Duration any time soon which requires libstd, and if this does come about we can always figure out a solution. In the meantime Duration also has a pro in that it's so simple, so it seems fine to include in libcore.

@clarcharr want to rebase before merging?

@rfcbot fcp merge

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 16, 2018

Member

Also can you move the tests to an the src/libcore/tests folder? The inline tests in the module won't be run I believe.

Member

alexcrichton commented Jan 16, 2018

Also can you move the tests to an the src/libcore/tests folder? The inline tests in the module won't be run I believe.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Jan 16, 2018

Member

rfcbot is not responding 😕

Member

kennytm commented Jan 16, 2018

rfcbot is not responding 😕

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 16, 2018

Member

er sorry that's my mistake, it was already done above!

@rfcbot resolved we-may-want-std

Member

alexcrichton commented Jan 16, 2018

er sorry that's my mistake, it was already done above!

@rfcbot resolved we-may-want-std

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 16, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Jan 16, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jan 24, 2018

Contributor

Just realised that one primary use of libstd Duration is to have a static method which times the duration a function takes to execute, and imo that would be better suited for the to-be-stabled Bencher from libtest

Contributor

clarcharr commented Jan 24, 2018

Just realised that one primary use of libstd Duration is to have a static method which times the duration a function takes to execute, and imo that would be better suited for the to-be-stabled Bencher from libtest

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 25, 2018

Member

That was an example of what we felt wasn't necessarily critical functionality to have but something we could stabilize at a later date via other means. How does that sound to you though?

Member

alexcrichton commented Jan 25, 2018

That was an example of what we felt wasn't necessarily critical functionality to have but something we could stabilize at a later date via other means. How does that sound to you though?

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jan 25, 2018

Contributor

Seems fine to me!

Contributor

clarcharr commented Jan 25, 2018

Seems fine to me!

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum Jan 25, 2018

Member

So I believe the consensus here is we want to merge this, but are waiting on a rebase. Is that correct?

Member

Mark-Simulacrum commented Jan 25, 2018

So I believe the consensus here is we want to merge this, but are waiting on a rebase. Is that correct?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 25, 2018

Member

Indeed!

Member

alexcrichton commented Jan 25, 2018

Indeed!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Jan 26, 2018

The final comment period is now complete.

rfcbot commented Jan 26, 2018

The final comment period is now complete.

@clarcharr

This comment has been minimized.

Show comment
Hide comment
@clarcharr

clarcharr Jan 29, 2018

Contributor

Rebased!

Contributor

clarcharr commented Jan 29, 2018

Rebased!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented Jan 30, 2018

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 30, 2018

Contributor

📌 Commit aab712c has been approved by alexcrichton

Contributor

bors commented Jan 30, 2018

📌 Commit aab712c has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 31, 2018

Contributor

⌛️ Testing commit aab712c with merge b8f2674...

Contributor

bors commented Jan 31, 2018

⌛️ Testing commit aab712c with merge b8f2674...

bors added a commit that referenced this pull request Jan 31, 2018

Auto merge of #46666 - clarcharr:duration_core, r=alexcrichton
Move Duration to libcore

Fixes #46520; should be merged after #46508.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 31, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b8f2674 to master...

Contributor

bors commented Jan 31, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b8f2674 to master...

@bors bors merged commit aab712c into rust-lang:master Jan 31, 2018

2 checks passed

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

@clarcharr clarcharr deleted the clarcharr:duration_core branch Apr 15, 2018

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