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

Implement Default for std::time::Duration #37546

Closed
luser opened this issue Nov 3, 2016 · 12 comments
Closed

Implement Default for std::time::Duration #37546

luser opened this issue Nov 3, 2016 · 12 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@luser
Copy link
Contributor

luser commented Nov 3, 2016

Duration doesn't implement Default. I found this out because I had a struct with #[derive(Default)] and I tried to add a Duration member to it, and that no longer worked. This means that I'd have to manually implement Default, which stinks because the struct has quite a few members. It seems like we could give Duration an implementation that just returns a zero-length Duration.

@vaartis
Copy link

vaartis commented Nov 3, 2016

This seems quite simple for a beginner, if everybody's ok with it, may i do it?

@alexcrichton
Copy link
Member

This was originally proposed in #32785 where when the libs team discussed it concluded that there's not a "clearly correct" default value for the Duration type.

That being said I'd personally be ok with just returning a zero duration, so I'd be curious if others from @rust-lang/libs could weigh in on this.

@sfackler
Copy link
Member

sfackler commented Nov 3, 2016

The zero duration seems as reasonable as a default as 0 does for integer types, so I'd be on board with this. If nothing else, given a Default impl for Duration, it's the only value that could possibly make sense.

@luser
Copy link
Contributor Author

luser commented Nov 3, 2016

I guess I can see that argument, like if you had:

struct SomeOptions {
  things: usize,
  timeout: Duration,
}

You probably wouldn't want to have timeout default to zero if you used #[derive(Default)]. However, I have a hard time believing that #[derive(Default)] would be the right choice for an options struct like that in the first place.

For APIs that take a timeout parameter, I suppose you could have an odd situation where people pass Default::default() or Duration::default(), but you could already have that with any parameter that implements Default, and I can't say I've come across it in practice. It seems more likely that an API would use Option<Duration> for such a thing, or a hand-written Default implementation.

@aturon
Copy link
Member

aturon commented Nov 4, 2016

I agree that if we have a default, 0 is the only sensible one to choose. It just continues to make me nervous that I can't think of many situations where that's actually the default you want for a duration -- but in any case where a default even makes sense, you should be using Option<Duration>.

@luser Out of curiosity, how is the duration being used in the struct that led you to this problem? And is 0 really the "right" default for your case?

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 4, 2016
@luser
Copy link
Contributor Author

luser commented Nov 5, 2016

@aturon I wanted to use it to accumulate a running duration of a set of events in a struct that accumulates stats:
https://github.com/luser/sccache2/blob/a7b5349da1d54ddba72f0640b0d388f0b9d61c00/src/server.rs#L121

Each event returns a Duration so I thought it'd be easy to just store one and accumulate it, but I didn't feel like writing the Default implementation by hand there, so I punted:
https://github.com/luser/sccache2/blob/a7b5349da1d54ddba72f0640b0d388f0b9d61c00/src/server.rs#L719

@aturon
Copy link
Member

aturon commented Nov 6, 2016

@luser Thanks, makes perfect sense!

TLDR: a good reason for a default duration is accumulation, and zero is the right starting point. And I can't really see any way in which this is a footgun (as we all keep saying, zero's the only value that could make sense anyway). So 👍.

However, given that there was some contention about this before, I'm going to run this change by the full libs team for approval:

@rfcbot fcp merge

@aturon
Copy link
Member

aturon commented Nov 6, 2016

@Tyannn Once the team has agreed we want to make this change, we'd be happy to take a PR from you implementing it!

@rfcbot
Copy link

rfcbot commented Nov 6, 2016

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

@brson brson added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Nov 10, 2016
@rfcbot
Copy link

rfcbot commented Nov 10, 2016

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

psst @aturon, I wasn't able to add the final-comment-period label, please do so.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 11, 2016
Discussed in rust-lang#37546 the libs team reached the conclusion that a default zero
duration seems like a reasonable implementation of the `Default` trait.

Closes rust-lang#37546
@alexcrichton
Copy link
Member

I've opened a PR for this at: #37699

eddyb added a commit to eddyb/rust that referenced this issue Nov 11, 2016
…r=brson

std: Derive `Default` for `Duration`.

Discussed in rust-lang#37546 the libs team reached the conclusion that a default zero
duration seems like a reasonable implementation of the `Default` trait.

Closes rust-lang#37546
eddyb added a commit to eddyb/rust that referenced this issue Nov 12, 2016
…r=brson

std: Derive `Default` for `Duration`.

Discussed in rust-lang#37546 the libs team reached the conclusion that a default zero
duration seems like a reasonable implementation of the `Default` trait.

Closes rust-lang#37546
@luser
Copy link
Contributor Author

luser commented Nov 14, 2016

Thanks for taking the time to work through this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. 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

8 participants