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

Allow more integer types when creating Instants #272

Closed
wants to merge 1 commit into from

Conversation

crawford
Copy link
Contributor

This allows any type that can be converted into an i64 to be used when
creating an Instant. Because this is no longer a concrete type, this
change may break existing code like the following:

error: attempt to multiply with overflow
   --> src/time.rs:282:37
    |
282 |         epoc = Instant::from_millis(2085955200 * 1000).into();
    |                                     ^^^^^^^^^^^^^^^^^
    |
    = note: #[deny(const_err)] on by default

This allows any type that can be converted into an `i64` to be used when
creating an Instant. Because this is no longer a concrete type, this
change may break existing code like the following:

    error: attempt to multiply with overflow
       --> src/time.rs:282:37
        |
    282 |         epoc = Instant::from_millis(2085955200 * 1000).into();
        |                                     ^^^^^^^^^^^^^^^^^
        |
        = note: #[deny(const_err)] on by default
@whitequark
Copy link
Contributor

You've demonstrated the downside of this PR (thanks!) but what are the concrete upsides?

@crawford
Copy link
Contributor Author

Instead of writing:

let timestamp = Instant::from_millis(rtc.cnt.read().cnt().bits().into());

I'll be able to write:

let timestamp = Instant::from_millis(rtc.cnt.read().cnt().bits());

It seems more common in the larger Rust ecosystem to perform the conversion in the function than at the callsite.

@whitequark
Copy link
Contributor

OK. It seems unlikely that there is a lot of code creating Instants from literals, and the fix is anyhow trivial.

@whitequark
Copy link
Contributor

@m-labs-homu r+

@m-labs-homu
Copy link

📌 Commit bd63e4e has been approved by whitequark

@m-labs-homu
Copy link

⌛ Testing commit bd63e4e with merge d381dcd...

@m-labs-homu
Copy link

☀️ Test successful - status-travis
Approved by: whitequark
Pushing d381dcd to master...

@crawford crawford deleted the instant branch January 28, 2019 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants