Implement `Default` for more types in the standard library by tbu- · Pull Request #32785 · rust-lang/rust

tbu- · 2016-04-07T00:13:13Z

Also add Hash to std::cmp::Ordering and most possible traits to
fmt::Error.

rust-highfive · 2016-04-07T00:13:18Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

tbu- · 2016-04-07T00:13:46Z

There was a reddit comment that motivated this (especially for Mutex and RwLock), but I can't find it anymore.

bluss · 2016-04-07T00:37:14Z

The added other trait impls also should be mentioned in the commit message or PR.

tbu- · 2016-04-07T09:44:46Z

@bluss Updated.

alexcrichton · 2016-04-07T16:26:57Z

Thanks @tbu-! Tagging with T-libs to ensure that this comes up during triage.

llogiq · 2016-04-08T01:04:44Z

My question on StackOverflow has brought up the concern that a 0-duration is not a sensible default in most cases. Using this e.g. as a timeout is bound to fail at runtime. It's probably better not to implement Default for Duration at all.

Why would it fail at runtime? The drawbacks were not fully explained on SO.

The problem is that Durations are often used as Timeouts and unless zero-duration timeouts are special case, the operation will always time out by default.

I don't have strong feelings about this, we can definitely remove this if this isn't wanted. Two upsides: Using 0 seems to have some precedence (integers, floats), even though it's not clear where you'll use them. Implementing Default for more types means that you can #[derive] it in more cases.

Yeah, that was exactly the argument I made in the reddit comment. And of course the default just has to make intuitive sense; we cannot foresee possible applications. Still, I'd like to have more input for this before we commit to an implementation, especially as insta-stability means we cannot easily back off.

llogiq · 2016-04-08T01:06:41Z

That reddit comment was by me. This would fix #31865 (and some more) by the way.

I'm also unsure if this should be directly stable.

llogiq · 2016-04-08T07:10:09Z

OK, so TIL trait impls are insta-stable. 😄

Apart from being unsure about the 'correct' default value for Duration (zero seems to make intuitive sense for timestamp arithmetic, but is a bad choice for timeouts), I'm in favor of this. It's more complete than #32807.

alexcrichton · 2016-04-15T00:03:09Z

The libs team discussed this during triage yesterday and the decision was that most of these seem fine but we should avoid doing something just for the sake of doing something. In line with that the default impls for Path and CStr seem like "just because", so can they be removed for now? Additionally, the Duration default seems like it's not "clearly the right choice" so can that be removed for now as well? (erring on the side of being conservative).

Other than that though this looked good to us!

tbu- · 2016-04-15T15:49:22Z

&CStr is pretty close to &str and Default is implemented for that type. Default is also implemented for &[T] and &mut [T].

The use case I have in mind here is when you have a Option<&CStr>, an unwrap_or_default should give you the empty string (like for Option<&str>).

Also add `Hash` to `std::cmp::Ordering` and most possible traits to `fmt::Error`.

llogiq · 2016-04-15T16:50:01Z

I also see no problem with CStr. Duration on the other hand should probably wait.

alexcrichton · 2016-04-15T17:09:33Z

Yeah splitting them out is fine, let's merge this and continue discussion on #32990

@bors: r+ 3df35a0

bors · 2016-04-16T03:26:19Z

⌛ Testing commit 3df35a0 with merge 6fa61b8...

Implement `Default` for more types in the standard library Also add `Hash` to `std::cmp::Ordering` and most possible traits to `fmt::Error`.

bors · 2016-04-16T06:34:37Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

llogiq · 2016-04-16T07:33:32Z

@tbu- 👍

llogiq · 2016-04-16T18:32:13Z

This fixes #31865.

bombless · 2016-04-26T04:47:27Z

I cannot think of a more proper default value for Duration other than zero though.

IMO if your scenario has a more proper value for a duration it should be a separate type and you may want to express duration itself through a trait.

And the implementation of Default might just be used casually, like https://github.com/bombless/to_default/blob/master/examples/simple.rs

rust-highfive assigned aturon Apr 7, 2016

tbu-force-pushed the pr_more_defaults branch from da08168 to b5b7fdbCompare April 7, 2016 09:44

alexcrichton added the T-libs-api [DEPRECATED; DO NOT USE] label Apr 7, 2016

brson mentioned this pull request Apr 7, 2016
impl Default for CondVar, Mutex and RwLock #32807
Closed

llogiq reviewed Apr 8, 2016
View reviewed changes

Implement Default for more types in the standard library
3df35a0
Also add `Hash` to `std::cmp::Ordering` and most possible traits to `fmt::Error`.

tbu-force-pushed the pr_more_defaults branch from b5b7fdb to 3df35a0Compare April 15, 2016 15:54

tbu- mentioned this pull request Apr 15, 2016
Add Default implementation for &CStr, CString, Path#32990
Merged

bors added a commit that referenced this pull request Apr 16, 2016
Auto merge of#32785- tbu-:pr_more_defaults, r=alexcrichton
6fa61b8
Implement `Default` for more types in the standard library Also add `Hash` to `std::cmp::Ordering` and most possible traits to `fmt::Error`.

bors merged commit 3df35a0 into rust-lang:masterApr 16, 2016

tbu- mentioned this pull request Apr 16, 2016
Missing Default for std::sync::Condvar #31865
Closed

alexcrichton mentioned this pull request Nov 3, 2016
Implement Default for std::time::Duration#37546
Closed

Uh oh!

Uh oh!

Conversation

tbu- commented Apr 7, 2016

Uh oh!

rust-highfive commented Apr 7, 2016

Uh oh!

tbu- commented Apr 7, 2016

Uh oh!

bluss commented Apr 7, 2016

Uh oh!

tbu- commented Apr 7, 2016

Uh oh!

alexcrichton commented Apr 7, 2016

Uh oh!

llogiqApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

blussApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

llogiqApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

tbu-Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

llogiqApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

llogiq commented Apr 8, 2016

Uh oh!

llogiq commented Apr 8, 2016

Uh oh!

alexcrichton commented Apr 15, 2016

Uh oh!

tbu- commented Apr 15, 2016

Uh oh!

llogiq commented Apr 15, 2016

Uh oh!

alexcrichton commented Apr 15, 2016

Uh oh!

bors commented Apr 16, 2016

Uh oh!

bors commented Apr 16, 2016

Uh oh!

llogiq commented Apr 16, 2016

Uh oh!

llogiq commented Apr 16, 2016

Uh oh!

bombless commented Apr 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants