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

Simplify checked_duration_since #59374

Merged
merged 3 commits into from Mar 26, 2019

Conversation

Projects
None yet
4 participants
@faern
Copy link
Contributor

faern commented Mar 22, 2019

This follows the same design as we updated to in #56490. Internally, all the system specific time implementations are checked, no panics. Then the panicking publicly exported API can just call the checked version of itself and make do with a single panic (expect) at the top.

Since the internal sys implementations are now checked, this gets rid of the extra if self >= &earlier check in checked_duration_since. Except likely making the generated machine code simpler, it also reduces the algorithm from "Check panic condition -> call possibly panicking method" to just "call non panicking method".

Added two test cases:

  • Edge case: Make sure checked_duration_since on two equal Instants produce a zero duration, not a None.
  • Most common/intended usage: Make sure later.checked_duration_since(earlier), returns an expected value.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 22, 2019

r? @shepmaster

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Mar 25, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 25, 2019

📌 Commit 1ccad16 has been approved by shepmaster

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Mar 25, 2019

/cc @alexcrichton as reviewer of the previous PR

Centril added a commit to Centril/rust that referenced this pull request Mar 26, 2019

Rollup merge of rust-lang#59374 - faern:simplify-checked-duration-sin…
…ce, r=shepmaster

Simplify checked_duration_since

This follows the same design as we updated to in rust-lang#56490. Internally, all the system specific time implementations are checked, no panics. Then the panicking publicly exported API can just call the checked version of itself and make do with a single panic (`expect`) at the top.

Since the internal sys implementations are now checked, this gets rid of the extra `if self >= &earlier` check in `checked_duration_since`. Except likely making the generated machine code simpler, it also reduces the algorithm from "Check panic condition -> call possibly panicking method" to just "call non panicking method".

Added two test cases:
* Edge case: Make sure `checked_duration_since` on two equal `Instant`s produce a zero duration, not a `None`.
* Most common/intended usage: Make sure `later.checked_duration_since(earlier)`, returns an expected value.

Centril added a commit to Centril/rust that referenced this pull request Mar 26, 2019

Rollup merge of rust-lang#59374 - faern:simplify-checked-duration-sin…
…ce, r=shepmaster

Simplify checked_duration_since

This follows the same design as we updated to in rust-lang#56490. Internally, all the system specific time implementations are checked, no panics. Then the panicking publicly exported API can just call the checked version of itself and make do with a single panic (`expect`) at the top.

Since the internal sys implementations are now checked, this gets rid of the extra `if self >= &earlier` check in `checked_duration_since`. Except likely making the generated machine code simpler, it also reduces the algorithm from "Check panic condition -> call possibly panicking method" to just "call non panicking method".

Added two test cases:
* Edge case: Make sure `checked_duration_since` on two equal `Instant`s produce a zero duration, not a `None`.
* Most common/intended usage: Make sure `later.checked_duration_since(earlier)`, returns an expected value.

bors added a commit that referenced this pull request Mar 26, 2019

Auto merge of #59431 - Centril:rollup, r=Centril
Rollup of 11 pull requests

Successful merges:

 - #59150 (Expand suggestions for type ascription parse errors)
 - #59232 (Merge `Promoted` and `Static` in `mir::Place`)
 - #59267 (Provide suggestion when using field access instead of path)
 - #59315 (Add no_hash to query macro and move some queries over)
 - #59334 (Update build instructions in README.md)
 - #59336 (Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js)
 - #59362 (Demo `FromIterator` short-circuiting)
 - #59374 (Simplify checked_duration_since)
 - #59389 (replace redundant note in deprecation warning)
 - #59410 (Clarify `{Ord,f32,f64}::clamp` docs a little)
 - #59419 (Utilize `?` instead of `return None`.)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Mar 26, 2019

Rollup merge of rust-lang#59374 - faern:simplify-checked-duration-sin…
…ce, r=shepmaster

Simplify checked_duration_since

This follows the same design as we updated to in rust-lang#56490. Internally, all the system specific time implementations are checked, no panics. Then the panicking publicly exported API can just call the checked version of itself and make do with a single panic (`expect`) at the top.

Since the internal sys implementations are now checked, this gets rid of the extra `if self >= &earlier` check in `checked_duration_since`. Except likely making the generated machine code simpler, it also reduces the algorithm from "Check panic condition -> call possibly panicking method" to just "call non panicking method".

Added two test cases:
* Edge case: Make sure `checked_duration_since` on two equal `Instant`s produce a zero duration, not a `None`.
* Most common/intended usage: Make sure `later.checked_duration_since(earlier)`, returns an expected value.

bors added a commit that referenced this pull request Mar 26, 2019

Auto merge of #59433 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #59150 (Expand suggestions for type ascription parse errors)
 - #59232 (Merge `Promoted` and `Static` in `mir::Place`)
 - #59267 (Provide suggestion when using field access instead of path)
 - #59315 (Add no_hash to query macro and move some queries over)
 - #59334 (Update build instructions in README.md)
 - #59362 (Demo `FromIterator` short-circuiting)
 - #59374 (Simplify checked_duration_since)
 - #59389 (replace redundant note in deprecation warning)
 - #59410 (Clarify `{Ord,f32,f64}::clamp` docs a little)
 - #59419 (Utilize `?` instead of `return None`.)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Mar 26, 2019

Auto merge of #59433 - Centril:rollup, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #59150 (Expand suggestions for type ascription parse errors)
 - #59232 (Merge `Promoted` and `Static` in `mir::Place`)
 - #59267 (Provide suggestion when using field access instead of path)
 - #59315 (Add no_hash to query macro and move some queries over)
 - #59334 (Update build instructions in README.md)
 - #59362 (Demo `FromIterator` short-circuiting)
 - #59374 (Simplify checked_duration_since)
 - #59389 (replace redundant note in deprecation warning)
 - #59410 (Clarify `{Ord,f32,f64}::clamp` docs a little)
 - #59419 (Utilize `?` instead of `return None`.)

Failed merges:

r? @ghost

@bors bors merged commit 1ccad16 into rust-lang:master Mar 26, 2019

@faern faern deleted the faern:simplify-checked-duration-since branch Mar 26, 2019

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.