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

Add checked_add method to Instant time type #56490

Merged
merged 4 commits into from Dec 14, 2018

Conversation

Projects
None yet
10 participants
@faern
Contributor

faern commented Dec 4, 2018

Appending functionality to the already opened topic of checked_add on time types over at #55940.

Doing checked addition between an Instant and a Duration is important to reliably determine a future instant. We could use this in the parking_lot crate to compute an instant when in the future to wake a thread up without risking a panic.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 4, 2018

r? @Kimundi

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

@faern

This comment has been minimized.

Contributor

faern commented Dec 4, 2018

I started by writing my code using ? rather than .and_then. I then started doing the same to related methods not really needed to add this PR functionality. But I felt that maybe using ? would be more idiomatic. I'm hoping both generates approximately the same machine code.

// Check that the checked_add_duration implementations seem approximately correct
let start = Instant::now();
thread::sleep(Duration::from_millis(500));

This comment has been minimized.

@faern

faern Dec 4, 2018

Contributor

I'm aware that time based tests are incredibly unreliable and usually not a good thing. But I felt that given the fact that the different implementations use time in vastly different scales it would be fairly easy to accidentally treat nanos as millis or similar. As such I thought something like this might help catch errors on the magnitude scale.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 10, 2018

Member

I wonder if perhaps you could do let end = start + Duration::from_millis(500)? I'm not sure that it's necessary to mix-in Instant::now() testing here perhaps?

This comment has been minimized.

@faern

faern Dec 10, 2018

Contributor

Then the sleep is of no use. The sleep + Instant::now() is the way to create two instants separated in time without using the add functionality of Instant. Or am I missing something?

The a +year test just a few lines above make sure the normal add and the checked add produce the same results.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 10, 2018

Member

Ah ok I think I see what you want to test, in that case I think it's fine to omit this test itself. It's true that Instant::now is notoriously hard to test correctly, so it's ok to just avoid it on this one!

This comment has been minimized.

@faern

faern Dec 10, 2018

Contributor

I removed that part of the test completely 👍

@faern faern force-pushed the faern:add-checked-add-to-instant branch from a16f8e4 to daf2aab Dec 4, 2018

@Dylan-DPC

This comment has been minimized.

Member

Dylan-DPC commented Dec 10, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned Kimundi Dec 10, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Dec 10, 2018

@Dylan-DPC I wish we had a better categorized list of reviewers. For example, I'm not a libs person, so I'm only really useful for some parts of the compiler + proc_macro.

r? @alexcrichton or @sfackler

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Dec 10, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 10, 2018

This looks great to me, thanks! Could the sys/* implementations actually only implement checked_add perhaps? And then the main add implementation is implemented in terms of that?

@faern faern force-pushed the faern:add-checked-add-to-instant branch from daf2aab to 6641501 Dec 10, 2018

@faern

This comment has been minimized.

Contributor

faern commented Dec 10, 2018

Moving some stuff out of sys/ was a great idea. It removed a lot of code. Did the same for the SystemTime::add_duration as well.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 10, 2018

Also while you're at it, want to add a SystemTime::checked_add too?

@faern

This comment has been minimized.

Contributor

faern commented Dec 10, 2018

I feel it would be the most consistent to do the exact same thing for subtraction. Add checked_sub to both Instant and SystemTime. But maybe leave that to a separate PR?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 10, 2018

Either way's fine by me, but given how our queue time isn't always the greatest adding them here seems fine by me

@faern faern force-pushed the faern:add-checked-add-to-instant branch from 6641501 to 5a6146f Dec 10, 2018

@faern

This comment has been minimized.

Contributor

faern commented Dec 10, 2018

Done. Added checked_sub for both time types under the same feature gate name. time_checked_add does not fit perfectly, but having two gates seemed overkill.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 10, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0549c3a3:start=1544484952341807474,finish=1544484953363552999,duration=1021745525
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:04:31]    Compiling libc v0.0.0 (/checkout/src/rustc/libc_shim)
[00:04:31]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:04:32]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:04:37]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:40] error[E0599]: no method named `checked_sub_duration` found for type `sys::unix::time::inner::Instant` in the current scope
[00:04:40]    --> src/libstd/time.rs:225:16
[00:04:40]     |
[00:04:40] 225 |         self.0.checked_sub_duration(&duration).map(|t| Instant(t))
[00:04:40]     | 
[00:04:40]    ::: src/libstd/sys/unix/time.rs:261:5
[00:04:40]     |
[00:04:40] 261 |     pub struct Instant {
[00:04:40] 261 |     pub struct Instant {
[00:04:40]     |     ------------------ method `checked_sub_duration` not found for this
[00:04:40]     = help: did you mean `checked_add_duration`?
[00:04:40] 
[00:04:40] 
[00:04:40] error[E0599]: no method named `checked_sub_duration` found for type `sys::unix::time::inner::SystemTime` in the current scope
[00:04:40]    --> src/libstd/time.rs:392:16
[00:04:40]     |
[00:04:40] 392 |         self.0.checked_sub_duration(&duration).map(|t| SystemTime(t))
[00:04:40]     | 
[00:04:40]    ::: src/libstd/sys/unix/time.rs:266:5
[00:04:40]     |
[00:04:40] 266 |     pub struct SystemTime {
[00:04:40] 266 |     pub struct SystemTime {
[00:04:40]     |     --------------------- method `checked_sub_duration` not found for this
[00:04:40]     = help: did you mean `checked_add_duration`?
[00:04:40] 
[00:04:40] 
[00:04:40] error[E0599]: no method named `sub_duration` found for type `sys::unix::time::Timespec` in the current scope
[00:04:40]     |
[00:04:40] 22  | struct Timespec {
[00:04:40] 22  | struct Timespec {
[00:04:40]     | --------------- method `sub_duration` not found for this
[00:04:40] ...
[00:04:40] 295 |             Instant { t: self.t.sub_duration(other) }
[00:04:40] 
[00:04:40] 
[00:04:40] error[E0599]: no method named `sub_duration` found for type `sys::unix::time::Timespec` in the current scope
[00:04:40]     |
[00:04:40] 22  | struct Timespec {
[00:04:40] 22  | struct Timespec {
[00:04:40]     | --------------- method `sub_duration` not found for this
[00:04:40] ...
[00:04:40] 323 |             SystemTime { t: self.t.sub_duration(other) }
[00:04:40] 
[00:04:40] error: aborting due to 4 previous errors
[00:04:40] 
[00:04:40] For more information about this error, try `rustc --explain E0599`.
[00:04:40] For more information about this error, try `rustc --explain E0599`.
[00:04:40] error: Could not compile `std`.
[00:04:40] 
[00:04:40] To learn more, run the command again with --verbose.
[00:04:40] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:40] expected success, got: exit code: 101
[00:04:40] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:40] Build completed unsuccessfully in 0:00:37
[00:04:40] Makefile:28: recipe for target 'all' failed
[00:04:40] make: *** [all] Error 1
15584 ./src/llvm/include
15212 ./src/llvm/include/llvm
15196 ./src/tools/lld
15056 ./src/test/run-pass
---
travis_time:end:06fa2a18:start=1544485243145643316,finish=1544485243153416429,duration=7773113
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:11c5ed3e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1ca86080
travis_time:start:1ca86080
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:06e00ae6
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@faern faern force-pushed the faern:add-checked-add-to-instant branch from 5a6146f to 3358de3 Dec 10, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 11, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2cb7d614:start=1544486003041141728,finish=1544486006705102857,duration=3663961129
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:11] 
[00:57:11] running 120 tests
[00:57:14] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii..............i...ii 100/120
[00:57:14] ..ii.i.....iiii.....
[00:57:14] 
[00:57:14]  finished in 3.490
[00:57:14] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:29] 
[00:57:29] running 118 tests
[00:57:53] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:57:57] ......iii.i.....ii
[00:57:57] 
[00:57:57]  finished in 28.347
[00:57:57] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@faern faern force-pushed the faern:add-checked-add-to-instant branch from 3358de3 to 442abbc Dec 11, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 11, 2018

@bors: r+

👍

@bors

This comment has been minimized.

Contributor

bors commented Dec 11, 2018

📌 Commit 442abbc has been approved by alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Dec 11, 2018

⌛️ Testing commit 442abbc with merge e4b6d8d...

@faern faern force-pushed the faern:add-checked-add-to-instant branch from 442abbc to 59496a9 Dec 13, 2018

@faern

This comment has been minimized.

Contributor

faern commented Dec 13, 2018

Fixed the build error.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 13, 2018

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

📌 Commit 59496a9 has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Dec 13, 2018

Rollup merge of rust-lang#56490 - faern:add-checked-add-to-instant, r…
…=alexcrichton

Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at rust-lang#55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.

bors added a commit that referenced this pull request Dec 13, 2018

Auto merge of #56775 - kennytm:rollup, r=kennytm
Rollup of 34 pull requests

Successful merges:

 - #56142 ([std] Osstr lossy example)
 - #56203 (Add lint for items deprecated in future)
 - #56343 (Remove not used mod)
 - #56351 (Stabilize `linker-flavor` flag.)
 - #56439 (Clearer error message for dead assign)
 - #56490 (Add checked_add method to Instant time type)
 - #56507 (polonius tweaks)
 - #56562 (Update libc version required by rustc)
 - #56600 (bootstrap: fix edition)
 - #56609 (Unconditionally emit the target-cpu LLVM attribute.)
 - #56637 (rustdoc: Fix local reexports of proc macros)
 - #56658 (Add non-panicking `maybe_new_parser_from_file` variant)
 - #56672 (Document time of back operations of a Linked List)
 - #56677 (#[must_use] on traits in stdlib)
 - #56679 (overhaul external doc attribute diagnostics)
 - #56691 (fix install broken link)
 - #56695 (Fix irrefutable matches on integer ranges)
 - #56699 (Use a `newtype_index!` within `Symbol`.)
 - #56702 ([self-profiler] Add column for percent of total time)
 - #56708 (Remove some unnecessary feature gates)
 - #56710 (Always set the RDRAND and RDSEED features on SGX)
 - #56713 (Test capacity of ZST vector)
 - #56718 (Use libbacktrace pretty-printing)
 - #56725 (fix rust-lang/rust issue #50583)
 - #56731 (Add missing urls in ffi module docs)
 - #56738 (Fix private_no_mangle_fns message grammar)
 - #56742 (infer: remove Box from a returned Iterator)
 - #56746 (Add test of current behavior (infer free region within closure body))
 - #56747 (target: remove Box returned by get_targets)
 - #56749 (x86: Add the `adx` target feature to whitelist)
 - #56755 (Account for `impl Trait` when suggesting lifetime)
 - #56756 (Disable btree pretty-printers on older gdbs)
 - #56758 (Add short emoji status to toolstate updates)
 - #56783 (Add `--pinentry-mode=loopback` to deployment script)

@faern faern force-pushed the faern:add-checked-add-to-instant branch from 59496a9 to 9e5e89a Dec 13, 2018

@faern

This comment has been minimized.

Contributor

faern commented Dec 13, 2018

This PR screwed the rollup. I have fixed the error from the log now. But I have not verified it will build on cloudabi, not sure how to do a cross platform check from my Linux machine.

@nikic

This comment has been minimized.

Contributor

nikic commented Dec 13, 2018

@faern You can use src/ci/docker/run.sh dist-various-2 to run the failing docker image locally.

@faern

This comment has been minimized.

Contributor

faern commented Dec 13, 2018

Thank you. This changes everything! :) I was going to dig into the travis config to figure out how to do that. But now I don't have to. I now also realize it's actually in the rustc guide if you just look hard enough, so I was apparently not RTFM:ing enough :)

Well, what's in this PR right now passes the dist-various-2 test at least

EDIT: dist-various-1 also passes. So I'm hoping this can pass now.

@nikic

This comment has been minimized.

Contributor

nikic commented Dec 13, 2018

Great :) Let's give this another try...

@bors r=alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

📌 Commit 9e5e89a has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018

Rollup merge of rust-lang#56490 - faern:add-checked-add-to-instant, r…
…=alexcrichton

Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at rust-lang#55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.

bors added a commit that referenced this pull request Dec 14, 2018

Auto merge of #56803 - kennytm:rollup, r=kennytm
Rollup of 36 pull requests

Successful merges:

 - #56203 (Add lint for items deprecated in future)
 - #56343 (Remove not used mod)
 - #56439 (Clearer error message for dead assign)
 - #56490 (Add checked_add method to Instant time type)
 - #56507 (polonius tweaks)
 - #56562 (Update libc version required by rustc)
 - #56600 (bootstrap: fix edition)
 - #56609 (Unconditionally emit the target-cpu LLVM attribute.)
 - #56658 (Add non-panicking `maybe_new_parser_from_file` variant)
 - #56672 (Document time of back operations of a Linked List)
 - #56677 (#[must_use] on traits in stdlib)
 - #56679 (overhaul external doc attribute diagnostics)
 - #56691 (fix install broken link)
 - #56695 (Fix irrefutable matches on integer ranges)
 - #56699 (Use a `newtype_index!` within `Symbol`.)
 - #56702 ([self-profiler] Add column for percent of total time)
 - #56708 (Remove some unnecessary feature gates)
 - #56709 (Remove unneeded extra chars to reduce search-index size)
 - #56710 (Always set the RDRAND and RDSEED features on SGX)
 - #56713 (Test capacity of ZST vector)
 - #56718 (Use libbacktrace pretty-printing)
 - #56725 (fix rust-lang/rust issue #50583)
 - #56731 (Add missing urls in ffi module docs)
 - #56738 (Fix private_no_mangle_fns message grammar)
 - #56742 (infer: remove Box from a returned Iterator)
 - #56744 (specialize: remove Boxes used by Children::insert)
 - #56746 (Add test of current behavior (infer free region within closure body))
 - #56747 (target: remove Box returned by get_targets)
 - #56749 (x86: Add the `adx` target feature to whitelist)
 - #56751 (Allow ptr::hash to accept fat pointers)
 - #56755 (Account for `impl Trait` when suggesting lifetime)
 - #56756 (Disable btree pretty-printers on older gdbs)
 - #56758 (Add short emoji status to toolstate updates)
 - #56760 (Deduplicate unsatisfied trait bounds)
 - #56769 (Add x86_64-unknown-uefi target)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)
@bors

This comment has been minimized.

Contributor

bors commented Dec 14, 2018

⌛️ Testing commit 9e5e89a with merge f4b07e0...

bors added a commit that referenced this pull request Dec 14, 2018

Auto merge of #56490 - faern:add-checked-add-to-instant, r=alexcrichton
Add checked_add method to Instant time type

Appending functionality to the already opened topic of `checked_add` on time types over at #55940.

Doing checked addition between an `Instant` and a `Duration` is important to reliably determine a future instant. We could use this in the `parking_lot` crate to compute an instant when in the future to wake a thread up without risking a panic.
@bors

This comment has been minimized.

Contributor

bors commented Dec 14, 2018

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

bors added a commit that referenced this pull request Dec 14, 2018

Auto merge of #56803 - kennytm:rollup, r=kennytm
Rollup of 36 pull requests

Successful merges:

 - #56203 (Add lint for items deprecated in future)
 - #56343 (Remove not used mod)
 - #56439 (Clearer error message for dead assign)
 - #56490 (Add checked_add method to Instant time type)
 - #56507 (polonius tweaks)
 - #56562 (Update libc version required by rustc)
 - #56600 (bootstrap: fix edition)
 - #56609 (Unconditionally emit the target-cpu LLVM attribute.)
 - #56658 (Add non-panicking `maybe_new_parser_from_file` variant)
 - #56672 (Document time of back operations of a Linked List)
 - #56677 (#[must_use] on traits in stdlib)
 - #56679 (overhaul external doc attribute diagnostics)
 - #56691 (fix install broken link)
 - #56695 (Fix irrefutable matches on integer ranges)
 - #56699 (Use a `newtype_index!` within `Symbol`.)
 - #56702 ([self-profiler] Add column for percent of total time)
 - #56708 (Remove some unnecessary feature gates)
 - #56709 (Remove unneeded extra chars to reduce search-index size)
 - #56710 (Always set the RDRAND and RDSEED features on SGX)
 - #56713 (Test capacity of ZST vector)
 - #56718 (Use libbacktrace pretty-printing)
 - #56725 (fix rust-lang/rust issue #50583)
 - #56731 (Add missing urls in ffi module docs)
 - #56738 (Fix private_no_mangle_fns message grammar)
 - #56742 (infer: remove Box from a returned Iterator)
 - #56744 (specialize: remove Boxes used by Children::insert)
 - #56746 (Add test of current behavior (infer free region within closure body))
 - #56747 (target: remove Box returned by get_targets)
 - #56749 (x86: Add the `adx` target feature to whitelist)
 - #56751 (Allow ptr::hash to accept fat pointers)
 - #56755 (Account for `impl Trait` when suggesting lifetime)
 - #56756 (Disable btree pretty-printers on older gdbs)
 - #56758 (Add short emoji status to toolstate updates)
 - #56760 (Deduplicate unsatisfied trait bounds)
 - #56769 (Add x86_64-unknown-uefi target)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)

@bors bors merged commit 9e5e89a into rust-lang:master Dec 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment