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

Tracking Issue for Duration::{zero, is_zero} (#![feature(duration_zero)]) #73544

Closed
5 tasks done
LukasKalbertodt opened this issue Jun 20, 2020 · 18 comments
Closed
5 tasks done
Labels
A-time Area: Time B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@LukasKalbertodt
Copy link
Member

LukasKalbertodt commented Jun 20, 2020

This is a tracking issue for Duration::{ZERO, is_zero} (#![feature(duration_zero)]). Proposed in RFC 2814 which got closed just because such a small change does not require an RFC. Proposed again and implemented in #72790.

impl Duration {
    pub const ZERO: Duration;
    pub const fn is_zero(&self) -> bool;
}

Steps

Unresolved questions:

  • Also add an associated constant ZERO? yes. done
  • Should the associated constant be preferred over the zero function? yes, zero() is removed.
@LukasKalbertodt LukasKalbertodt added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 20, 2020
@tesuji

This comment has been minimized.

@LukasKalbertodt LukasKalbertodt changed the title Tracking Issue for XXX Tracking Issue for Duration::{zero, is_zero} (#![feature(duration_zero)]) Jun 20, 2020
@LukasKalbertodt

This comment has been minimized.

@KodrAus KodrAus added Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@MightyPork
Copy link

Are there plans for stabilization?

I had these two traits in my code for over a year, and they now show the "unstable_name_collisions" lint every build, everywhere they're used - #[allow(unstable_name_collisions)] does not work well with a workspace, I have to put it in all modules or rename the functions :/

/// Extensions for `Duration`
pub trait DurationExt : Copy {
    /// Returns true if self is equivalent to zero
    fn is_zero(self) -> bool;

    /// Get a zero duration
    fn zero() -> Duration {
        Duration::new(0, 0)
    }
}

impl DurationExt for Duration {
    #[inline]
    fn is_zero(self) -> bool {
        self.as_secs() == 0 && self.subsec_nanos() == 0
    }
}

@jonhoo
Copy link
Contributor

jonhoo commented Sep 11, 2020

My guess is that these can probably be stabilized as-is?

@LukasKalbertodt
Copy link
Member Author

As the top comments says, the only open question is basically whether we want the associated const ZERO instead of the two methods.

// Do we want this?
let d = Duration::zero();
if d.is_zero() { ... }

// Or rather this?
let d = Duration::ZERO;
if d == Duration::ZERO { ... }

There are certainly some uses of associated consts in std already:

  • Ip4Addr::{LOCALHOST, BROADCAST, UNSPECIFIED}. Ip4Addr also has plenty of is_* methods, including is_broadcast. It does not have localhost, .. constructors functions.
  • Integers and floats have associated consts and no constructor functions.

I think both of those two solutions (or any mix of those) would be fine. I don't think anyone would consider either of those a "bad API". So if you think that either one of those should be stabilized, go ahead and either open a PR that adds the const ZERO or a PR that stabilizes the existing methods. In either case, provide some reasoning why the API you're suggesting is good/better.

@MightyPork
Copy link

I was thinking about the pros and cons, and I'm really divided, though leaning slightly towards ZERO

Duration::zero()

  • Intuitive (demonstrated by my independently invented trait with the same methods)
  • Might be more obvious to beginners who don't know that Duration is Copy - it looks like a constructor
  • Can be used in places that accept Fn() -> Duration - I'm not sure how useful that really is
  • Optimized to a constant = no overhead

Duration::ZERO

  • Cheap and straightforward, it is a constant
  • Name clearly indicates it's a constant
  • A bit shorter
  • Has a precedent in Ip4Addr

Regardless, is_zero() is a useful addition - although you can use d == Duration::ZERO, calling a method is much more succinct and makes for cleaner code

@Rua
Copy link
Contributor

Rua commented Sep 30, 2020

My preference is for the constant. Places that need a function can easily be given || Duration::ZERO, which doesn't seem like much of a bother.

@workingjubilee
Copy link
Member

As of #78216, Nightly Rust moves to the constant for a variety of reasons stated here, in addition to the fact that a const does not require a const evaluation context in order to be used in places where consts but not dynamic values are accepted... this matters a lot for match contexts and the many sugared versions of match.

@KodrAus KodrAus added the A-time Area: Time label Jan 6, 2021
@TehPers
Copy link

TehPers commented Mar 11, 2021

Are there any open questions remaining, or does this just need a stabilization PR now?

@m-ou-se
Copy link
Member

m-ou-se commented Mar 31, 2021

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Mar 31, 2021

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 31, 2021
@rfcbot
Copy link

rfcbot commented Mar 31, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 31, 2021
@workingjubilee
Copy link
Member

workingjubilee commented Apr 10, 2021

Well, I suppose it is a final comment period:

I do not particularly feel that is_zero is necessary, because most numeric types do not have is_zero methods either, and broadly I would not want them to acquire such. In #78216 I omitted removal because there was at least one request in favor. Nonetheless, I don't particularly stand by it: even if one specifically wanted a method, it seems like unnecessary duplication next to

use std::time::Duration;
use std::cmp::PartialEq;
use fun_library::some_duration;

fn main() {
    let d = some_duration();
    if d.eq(&Duration::ZERO) {
        println!("what could be less than an instant?");
    }
}

But neither am I strongly against if it is felt that there is something different about Duration that justifies such. That said, every additional line in documentation makes it harder to find other lines in documentation, so I still feel it should justify itself.

@BurntSushi
Copy link
Member

@workingjubilee I guess I don't really see Duration as a strictly numeric type. It has a bit of a higher level meaning than that. So I don't think the precedent with integer types is precisely relevant here.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 10, 2021
@rfcbot
Copy link

rfcbot commented Apr 10, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@workingjubilee
Copy link
Member

@workingjubilee I guess I don't really see Duration as a strictly numeric type. It has a bit of a higher level meaning than that. So I don't think the precedent with integer types is precisely relevant here.

That seems fair, then, if the precedent is not considered to hold in either direction.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 11, 2021

Without .is_zero(), you'd have to import Duration or write == std::time::Duration::ZERO, which is a bit much for such a simple operation. And we already have things like is_empty() on containers and quite a few more is_..() functions like that.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 12, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 13, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Apr 22, 2021

Stabilized in #84084.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-time Area: Time B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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. Libs-Small Libs issues that are considered "small" or self-contained Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests