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

Add functions Duration::try_from_secs_{f32, f64} #82179

Merged
merged 2 commits into from
Jun 15, 2021

Conversation

mbartlett21
Copy link
Contributor

@mbartlett21 mbartlett21 commented Feb 16, 2021

These functions allow constructing a Duration from a floating point value that could be out of range without panicking.

Tracking issue: #83400

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@marmeladema
Copy link
Contributor

Just a random contributor opinon here^^.
If the try_ prefix is usually associated with a Result return type then maybe checked_from_secs would work with an Option return type? I feel like if there are different reasons for the failure (or if we suspect their will be in the future), then a Result return type and a proper error being returned is nicer but if it's just a blind failure then a Option is fine.

@jieyouxu
Copy link
Member

Maybe it would make sense then to have both checked_from_secs_{f32,f64} -> Option<Duration> and try_from_secs_{f32,f64} -> Result<Duration, DurationConversionError> (the checked variant for wishing to handle fallible conversion but don't care for the cause, the try variant for wishing to handle fallible conversion and to know the cause of the conversion failure)? Where DurationConversionError is something like

enum DurationConversionError {
    NotFinite, // note: maybe distinguish between Inf/-Inf and NaN floating point values?
    Negative,
    Overflow
}

@marmeladema
Copy link
Contributor

Well, if we do want a get the actual error and if the error is not especially costly to compute, which is most likely going to be the case, I don't really see the benefit of having another version that returns an Option.
A user of that API can choose to ignore the error, call .ok() to turn into an Option or whatever.

@mbartlett21
Copy link
Contributor Author

Should I open a tracking issue where this can be discussed?

I would rather have an opaque error type that just displays different messages for different errors, only using an enum internally. (See ParseIntError for an example)

@crlf0710 crlf0710 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 5, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2021
@Dylan-DPC-zz
Copy link

@mbartlett21

Should I open a tracking issue where this can be discussed?

yes it is better to do that.

@mbartlett21
Copy link
Contributor Author

The tracking issue is #83400

@mbartlett21 mbartlett21 changed the title Add functions Duration::try_from_secs_{f32, f64} Add functions Duration::checked_from_secs_{f32, f64} Mar 23, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@mbartlett21 mbartlett21 changed the title Add functions Duration::checked_from_secs_{f32, f64} Add functions Duration::try_from_secs_{f32, f64} Mar 23, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2021
@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2021
@mbartlett21
Copy link
Contributor Author

@rustbot modify labels +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 22, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2021
@JohnTitor
Copy link
Member

r? @joshtriplett
and @mbartlett21 it'd be great if you could squash commits, the current is a bit long to read.

This also adds the error type used, `FromSecsError` and its `impl`s.
`Duration::from_secs_{f32, f64}` now use the results from the
non-panicking functions and unwrap it.
@mbartlett21
Copy link
Contributor Author

@JohnTitor I have made it into two commits now, one adding the new functions, and the other changing from_secs_{f32, f64} to use the new functions.

@joshtriplett
Copy link
Member

Posted one comment; LGTM otherwise. r=me with that fixed.

@joshtriplett
Copy link
Member

@bors r+

@JohnTitor JohnTitor closed this Jun 15, 2021
@JohnTitor JohnTitor reopened this Jun 15, 2021
@JohnTitor
Copy link
Member

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Jun 15, 2021

📌 Commit 7803955 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#80269 (Explain non-dropped sender recv in docs)
 - rust-lang#82179 (Add functions `Duration::try_from_secs_{f32, f64}`)
 - rust-lang#85608 (Stabilize `ops::ControlFlow` (just the type))
 - rust-lang#85792 (Refactor windows sockets impl methods)
 - rust-lang#86220 (Improve maybe_uninit_extra docs)
 - rust-lang#86277 (Remove must_use from ALLOWED_ATTRIBUTES)
 - rust-lang#86285 (:arrow_up: rust-analyzer)
 - rust-lang#86294 (Stabilize {std, core}::prelude::rust_*.)
 - rust-lang#86306 (Add mailmap entries for myself)
 - rust-lang#86314 (Remove trailing triple backticks in `mut_keyword` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jun 15, 2021

⌛ Testing commit 7803955 with merge 607d6b0...

@bors bors merged commit 1e14d39 into rust-lang:master Jun 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 15, 2021
@jplatte jplatte mentioned this pull request Jun 18, 2021
65 tasks
@mbartlett21 mbartlett21 deleted the patch-5 branch May 30, 2022 06:19
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Oct 24, 2022
…on-try-from-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang#83400.

### Implementation History

- rust-lang#82179
- rust-lang#90247
- rust-lang#96051
- Changed error type to `FromFloatSecsError` in rust-lang#90247
- rust-lang#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue rust-lang#72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

`@rustbot` label +T-libs-api -T-libs

cc `@mbartlett21`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 24, 2022
…on-try-from-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang#83400.

### Implementation History

- rust-lang#82179
- rust-lang#90247
- rust-lang#96051
- Changed error type to `FromFloatSecsError` in rust-lang#90247
- rust-lang#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue rust-lang#72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

``@rustbot`` label +T-libs-api -T-libs

cc ``@mbartlett21``
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
…om-secs-float, r=dtolnay

Stabilize `duration_checked_float`

## Stabilization Report

This stabilization report is for a stabilization of `duration_checked_float`, tracking issue: rust-lang/rust#83400.

### Implementation History

- rust-lang/rust#82179
- rust-lang/rust#90247
- rust-lang/rust#96051
- Changed error type to `FromFloatSecsError` in rust-lang/rust#90247
- rust-lang/rust#96051 changes the rounding mode to round-to-nearest instead of truncate.

## API Summary

This stabilization report proposes the following API to be stabilized in `core`, along with their re-exports in `std`:

```rust
// core::time

impl Duration {
    pub const fn try_from_secs_f32(secs: f32) -> Result<Duration, TryFromFloatSecsError>;
    pub const fn try_from_secs_f64(secs: f64) -> Result<Duration, TryFromFloatSecsError>;
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TryFromFloatSecsError { ... }

impl core::fmt::Display for TryFromFloatSecsError { ... }
impl core::error::Error for TryFromFloatSecsError { ... }
```

These functions are made const unstable under `duration_consts_float`, tracking issue #72440.

There is an open question in the tracking issue around what the error type should be called which I was hoping to resolve in the context of an FCP.

In this stabilization PR, I have altered the name of the error type to `TryFromFloatSecsError`. In my opinion, the error type shares the name of the method (adjusted to accommodate both types of floats), which is consistent with other error types in `core`, `alloc` and `std` like `TryReserveError` and `TryFromIntError`.

## Experience Report

Code such as this is ready to be converted to a checked API to ensure it is panic free:

```rust
impl Time {
    pub fn checked_add_f64(&self, seconds: f64) -> Result<Self, TimeError> {
        // Fail safely during `f64` conversion to duration
        if seconds.is_nan() || seconds.is_infinite() {
            return Err(TzOutOfRangeError::new().into());
        }

        if seconds.is_sign_positive() {
            self.checked_add(Duration::from_secs_f64(seconds))
        } else {
            self.checked_sub(Duration::from_secs_f64(-seconds))
        }
    }
}
```

See: artichoke/artichoke#2194.

`@rustbot` label +T-libs-api -T-libs

cc `@mbartlett21`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust-2018-preview Area: The 2018 edition preview disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.