Skip to content

Ensure two SystemTimes are equal when nanos add to exactly 1B #30173

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

Merged
merged 4 commits into from
Dec 4, 2015

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Dec 2, 2015

Currently if you add a duration which should lead to 0 nanos and 1
additional second, we end up with no additional seconds, and 1000000000
nanos.

Currently if you add a duration which should lead to 0 nanos and 1
additional second, we end up with no additional seconds, and 1000000000
nanos.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sfackler
Copy link
Member

sfackler commented Dec 2, 2015

Some tidy errors: https://travis-ci.org/rust-lang/rust/builds/94495543#L509

Looks good to me other than that though.

@alexcrichton
Copy link
Member

Gah oops, thanks @sgrif!

@sgrif
Copy link
Contributor Author

sgrif commented Dec 2, 2015

Made tidy!

sgrif added a commit to diesel-rs/diesel that referenced this pull request Dec 2, 2015
Hopefully I can remove this tomorrow, but I don't want a red build until
then.
@alexcrichton
Copy link
Member

@bors: r+ a59cd36

@bors
Copy link
Collaborator

bors commented Dec 3, 2015

⌛ Testing commit a59cd36 with merge 354f74e...

@bors
Copy link
Collaborator

bors commented Dec 3, 2015

💔 Test failed - auto-mac-64-nopt-t

@sgrif
Copy link
Contributor Author

sgrif commented Dec 3, 2015

Ah right, the added test is invalid on mac/ios, as it doesn't have nanosecond precision. Does it make sense to just cfg it out on that platform?

Those platforms don't support nanosecond precision, so adding 1
nanosecond does nothing.
@sgrif
Copy link
Contributor Author

sgrif commented Dec 3, 2015

Tests should be passing on all platforms now

@sfackler
Copy link
Member

sfackler commented Dec 3, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 3, 2015

📌 Commit 5dbc373 has been approved by sfackler

@bors
Copy link
Collaborator

bors commented Dec 4, 2015

⌛ Testing commit 5dbc373 with merge 2a33a32...

@bors
Copy link
Collaborator

bors commented Dec 4, 2015

💔 Test failed - auto-mac-64-nopt-t

@sgrif
Copy link
Contributor Author

sgrif commented Dec 4, 2015

Well, there is still a completely valid bug here on Mac. I will fix in the
morning.

On Thu, Dec 3, 2015, 6:42 PM bors [email protected] wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/7307

Reply to this email directly or view it on GitHub
#30173 (comment).

@alexcrichton
Copy link
Member

I think the same >= (instead of >) test may be necessary here as well.

@sgrif
Copy link
Contributor Author

sgrif commented Dec 4, 2015

Updated.

@alexcrichton
Copy link
Member

@bors: r+ 0747142

@bors
Copy link
Collaborator

bors commented Dec 4, 2015

⌛ Testing commit 0747142 with merge 55a4e05...

bors added a commit that referenced this pull request Dec 4, 2015
Currently if you add a duration which should lead to 0 nanos and 1
additional second, we end up with no additional seconds, and 1000000000
nanos.
@bors bors merged commit 0747142 into rust-lang:master Dec 4, 2015
@sgrif sgrif deleted the sg-fix-time-bug branch December 4, 2015 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants