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

Adding is_zero() to core::time::Duration #2814

Closed
wants to merge 4 commits into from

Conversation

@InTVyWOC
Copy link

InTVyWOC commented Nov 12, 2019

This PR is for adding new function is_zero() to core::time::Duration.

Related issue: #2809

InTVyWOC added 2 commits Nov 12, 2019
@InTVyWOC InTVyWOC marked this pull request as ready for review Nov 12, 2019
@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 12, 2019

Sort of prior art, albeit in progress and unreleased: I'm rewriting the time crate, and included this exact method. It is slightly different in that the implementation allows for both positive and negative durations, and there are corresponding methods.

I fully support, though! Even though I don't think an RFC is necessary for something like this :)

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 13, 2019

A couple of notes:

  • I would not in general expect == time::ZERO to generate more code than the is_zero method, especially if == gets inlined. I do think that .is_zero() seems more readable, though, and I still think we should have an .is_zero() method, but I don't think we need it for performance reasons specifically.

  • Another alternative: we could have a constant function Duration::zero() that generates a zero duration. Worth mentioning as an alternative, I think.

@LukasKalbertodt

This comment has been minimized.

Copy link
Contributor

LukasKalbertodt commented Nov 13, 2019

👍 On adding is_zero. Seems like a small and useful addition to me.

@joshtriplett mentioned Duration::zero() as alternative. I would like to propose it as a potential addition. I.e. let's just add Duration::is_zero and Duration::zero.

And as mentioned in the issue you opened, it would have probably been fine to directly create a PR on the main repo. It's a really small change.

- Added `zero()` as one alternative and future possibility.
- Removed some drawbacks of `ZERO` alternative.
@InTVyWOC

This comment has been minimized.

Copy link
Author

InTVyWOC commented Nov 14, 2019

@LukasKalbertodt

Thanks. I've updated the file.

I will create new PR if this RFC would be changed to "final comment period" state (like in guideline). It would prevent a lot of changes from time to time, while this RFC is being updated.

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Dec 5, 2019

This really doesn't have to go through the full RFC process, a PR on rust-lang/rust is just fine. Closing in favor of that.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Dec 5, 2019

@jonas-schievink #2809 can be closed as well, then, I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.