Skip to content

/rust

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

time: Deprecate the library in the distribution #18858

Merged
merged 1 commit into from Nov 13, 2014

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@alexcrichton @rust-highfive @pnkfelix @huonw @bors
@alexcrichton
Copy link
Member

alexcrichton commented Nov 10, 2014

This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
libtime crate as-is today should add this to their Cargo manifests:

[dependencies.time]
git = "https://github.com/rust-lang/time"

To implement this transition, a new function Duration::span was added to the
std::time::Duration time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with FnOnce unboxed closures as moving in and out will be a little
easier.

Due to the deprecation of the in-tree crate, this is a:

[breaking-change]

cc #18855, some of the conversions in the src/test/bench area may have been a
little nicer with that implemented
@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Nov 10, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@alexcrichton alexcrichton referenced this pull request Nov 10, 2014

Closed

Rename tm_gmtoff to tm_utcoff #18832

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Nov 10, 2014

cc #18585, one less crate to worry about!
src/libstd/time/duration.rs
@@ -135,6 +135,14 @@ impl Duration {
Duration { secs: secs, nanos: nanos as i32 }
}

/// Runs a closure, returning the duration of time it took to run the
/// closure.
pub fn span(f: ||) -> Duration {

This comment has been minimized.

Sign in to view
Copy link
@ghost

ghost Nov 11, 2014

I wouldn't want to block this PR on this but I think it'd be better for span to take a closure that returns T and return a tuple of (Duration, T) to make it more general-purpose. It'd certainly make it easier to avoid the extra mutable variables that are introduced at this function's call sites in this PR.

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Nov 11, 2014

Author Member

Yeah that would definitely help out those use cases, but I figured that it would be slightly odd to have to worry about the tuple afterwards. I think it may be the right way to go though, it seemed to come up more often than I anticipated!
@ghost

This comment has been minimized.

Sign in to view
Copy link

ghost commented Nov 11, 2014

As you're saying we'll tweak the interface with unboxed closures anyway, at which point we can make the suggested change.

@alexcrichton alexcrichton force-pushed the alexcrichton:remove-time branch from 5ed19a0 to 4f63687 Nov 12, 2014

bors added a commit that referenced this pull request Nov 12, 2014

@bors
auto merge of #18858 : alexcrichton/rust/remove-time, r=jakub 
This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
`libtime` crate as-is today should add this to their Cargo manifests:

    [dependencies.time]
    git = "https://github.com/rust-lang/time"

To implement this transition, a new function `Duration::span` was added to the
`std::time::Duration` time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with `FnOnce` unboxed closures as moving in and out will be a little
easier.

Due to the deprecation of the in-tree crate, this is a:

[breaking-change]

cc #18855, some of the conversions in the `src/test/bench` area may have been a
little nicer with that implemented
fbf508a

@alexcrichton alexcrichton force-pushed the alexcrichton:remove-time branch from 4f63687 to a90f44c Nov 12, 2014

bors added a commit that referenced this pull request Nov 12, 2014

@bors
auto merge of #18858 : alexcrichton/rust/remove-time, r=jakub 
This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
`libtime` crate as-is today should add this to their Cargo manifests:

    [dependencies.time]
    git = "https://github.com/rust-lang/time"

To implement this transition, a new function `Duration::span` was added to the
`std::time::Duration` time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with `FnOnce` unboxed closures as moving in and out will be a little
easier.

Due to the deprecation of the in-tree crate, this is a:

[breaking-change]

cc #18855, some of the conversions in the `src/test/bench` area may have been a
little nicer with that implemented
4b0e0a7
@alexcrichton
time: Deprecate the library in the distribution 
This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
`libtime` crate as-is today should add this to their Cargo manifests:

    [dependencies.time]
    git = "https://github.com/rust-lang/time"

To implement this transition, a new function `Duration::span` was added to the
`std::time::Duration` time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with `FnOnce` unboxed closures as moving in and out will be a little
easier.

Due to the deprecation of the in-tree crate, this is a:

[breaking-change]

cc #18855, some of the conversions in the `src/test/bench` area may have been a
little nicer with that implemented
Loading status checks…
fcd05ed

@alexcrichton alexcrichton force-pushed the alexcrichton:remove-time branch from a90f44c to fcd05ed Nov 12, 2014

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Owner Author

alexcrichton commented on fcd05ed Nov 12, 2014

r=jakub-
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented on fcd05ed Nov 12, 2014

saw approval from jakub
at alexcrichton@fcd05ed

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Nov 12, 2014

merging alexcrichton/rust/remove-time = fcd05ed into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Nov 12, 2014

alexcrichton/rust/remove-time = fcd05ed merged ok, testing candidate = 5745e41

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors replied Nov 13, 2014

fast-forwarding master to auto = 5745e41

bors added a commit that referenced this pull request Nov 12, 2014

@bors
auto merge of #18858 : alexcrichton/rust/remove-time, r=jakub 
This commit deprecates the entire libtime library in favor of the
externally-provided libtime in the rust-lang organization. Users of the
`libtime` crate as-is today should add this to their Cargo manifests:

    [dependencies.time]
    git = "https://github.com/rust-lang/time"

To implement this transition, a new function `Duration::span` was added to the
`std::time::Duration` time. This function takes a closure and then returns the
duration of time it took that closure to execute. This interface will likely
improve with `FnOnce` unboxed closures as moving in and out will be a little
easier.

Due to the deprecation of the in-tree crate, this is a:

[breaking-change]

cc #18855, some of the conversions in the `src/test/bench` area may have been a
little nicer with that implemented
Loading status checks…
5745e41

@bors bors closed this Nov 13, 2014

@bors bors merged commit fcd05ed into rust-lang:master Nov 13, 2014
2 checks passed

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default all tests passed

@ben0x539 ben0x539 referenced this pull request Nov 13, 2014

Closed

std::time::duration::Duration::span() is backwards #18925

This was referenced Nov 13, 2014

Closed
Merged

@alexcrichton alexcrichton deleted the alexcrichton:remove-time branch Nov 20, 2014

@aturon aturon referenced this pull request Nov 24, 2014

Closed

Stabilization metabug: 1.0-alpha #19260

140 of 142 tasks complete
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix commented on src/libstd/time/mod.rs in fcd05ed Feb 25, 2015

fyi @alexcrichton i think this multiply is overflowing. In all branches; its just that the arithmetic-overflow branch is now catching it happening.

Obviously the multiply does not live here anymore; it has AFAICT been migrated to std::sys::windows::time

The only evidence I have currently of the overflow is here: try build log
(scroll to the bottom, you'll see at the end the following message:)

thread 'rustc' panicked at 'arithmetic operation overflowed',
C:\bot\slave\try-win-32\build\src\libstd\sys\windows\time.rs:27

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix replied Feb 25, 2015

BTW I mention this only because I thought it was an interesting instance of overflow cropping up. I am using the following reformulation to avoid the overflow (hopefully): pnkfelix@b6d2ccb

But I would be curious to know your thoughts as to what possible fallout there is from the overflow here. I have not yet attempted to observe how much it may or may not be messing up the result of fn ns().

This comment has been minimized.

Sign in to view
Copy link
Member

huonw replied Feb 25, 2015

cc #22788

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton replied Feb 25, 2015

Oh dear that's not good! Looks like @vadimcn is on the case though with the PR @huonw mentioned

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix replied Feb 25, 2015

It looks like @vadimcn's approach is just to divide before doing the multiply; this certainly avoids the overflow, but also throws away information.

I believe my technique avoids losing the least significant digits in a portable way.

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix replied Feb 25, 2015

(I apologize, the previous comment was written before i had had my first cup of coffee. :( )
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.