Make #[derive(Copy, Eq, Ord)] imply #[derive(Clone, PartialEq, PartialOrd)] by erickt · Pull Request #23905 · rust-lang/rust

erickt · 2015-03-31T21:44:19Z

This reduces the amount of boilerplate users need to do in order to derive some common traits.

This is related to #23860.

rust-highfive · 2015-03-31T21:44:30Z

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

sfackler · 2015-03-31T21:50:47Z

The derived partial_cmp can be optimized in this case to just Some(self.cmp(other)).
EDIT: Hmm, though they're not totally equivalent in the presence of type paramters unfortunately.

Good point. I've got a build running that applies this PartialOrd optimization to non-generic types. I'll push it up when it finishes running.

aturon · 2015-03-31T22:12:01Z

🎯

alexcrichton · 2015-03-31T23:28:40Z

Out of curiosity, is it still possible to have #[derive(Ord)] but a manual implementation of PartialOrd?

sfackler · 2015-03-31T23:31:49Z

@alexcrichton I think the more common thing would be the opposite since PartialOrd has a trivial impl if you've already implemented Ord::cmp.

alexcrichton · 2015-03-31T23:36:06Z

Hm true. In general I've been very worried in the past about this form of deriving as it seems like a footgun for various forms of configuration of deriving. For example it looks like instances of derive(Eq) with a manual PartialEq impl were removed in favor of a manual Eq impl. I worry about the "half-derive" case, but it may not be too large of a worry in practice. For example adding an impl of Eq is just a marker trait so it's not so bad, and I'm not sure how much this applies to Ord and PartialOrd...

erickt · 2015-04-01T01:46:19Z

@alexcrichton: with this PR we could no longer provide a manual implementation of PartialOrd with #[derive(Ord)]. Is there ever a case where we'd want a manual implementation if we already have version of Ord? That also seems like it could be a source of bugs if the two were inconsistent.

nikomatsakis · 2015-04-01T12:43:21Z

My main concern is whether this PR requires an RFC. It seems to be at least mildly controversial. Obviously this would have to land by 1.0 though! (I personally acknowledge @alexcrichton's concerns, but think that this PR is a net improvement on the current situation.)

erickt · 2015-04-01T17:59:09Z

@sfackler: I just pushed up a patch that simplifies generating PartialOrd for non-generic Ord types. I'm not sure if my metrics are right, but this PR appears to be shaving off about 100KB from libstd.dylib when compared with the latest nightly libstd.dylib, but I can't say if that's all due to this PR or some other optimizations going on. I could do a fresh build to compare the too if we want a real number.

nikomatsakis · 2015-04-01T20:29:42Z

r+ from me (modulo RFC) but I'm not really that familiar with this code. Certainly I'd like to hear @sfackler or @huonw weigh in.

bors · 2015-04-02T10:05:34Z

☔ The latest upstream changes (presumably #23963) made this pull request unmergeable. Please resolve the merge conflicts.

erickt · 2015-04-02T16:25:45Z

I rebased on top of HEAD to avoid some merge conflicts and squashed some of the cleanup commits together.

This allows #[derive(...)]` to create more than one impl

This PR generates a simplified `PartialOrd` implementation when the type is `Ord` and non-generic. It produces essentially: impl ::std::cmp::PartialOrd for $ty { fn partial_cmp(&self, other: &$ty) -> Option<std::cmp::Ordering> { Some(::std::cmp::Ord::cmp(self, other)) } }

huonw · 2015-04-03T07:02:27Z

Copying this comment to ensure it doesn't get lost: I believe it one should be able to make the simple-impls work via the self_args fields of Substructure.

Also, another minor point: what about supporting #[derive(Copy, Clone)] struct Foo; (i.e. explicitly write the Clone?)?

Alsoalso, I wonder about the case of optimising PartialOrd (e.g. maybe < can be computed faster than a full comparison). There's a big cliff between:

#[derive(Ord)]structFoo{ ... }

and

structFoo{ ... }implOrdforFoo{fncmp(&self,other:&Foo) -> Ordering{// ... lots of stuff ...}}implPartialOrdforFoo{fnpartial_cmp(&self,other:&Foo) -> Option<Ordering>{Some(self.cmp(other))}fnlt(&self,other:&Foo) -> bool{/* supafast */}}

That said, it seems like this PR is the right default, and we can add an opt-out mechanism in future.

nikomatsakis · 2015-04-03T09:51:16Z

Also, another minor point: what about supporting #[derive(Copy, Clone)] struct Foo; (i.e. explicitly write the Clone?)?

I thought the PR did permit this, albeit with a FIXME attached? (I tend to think we should just allow it and, at most, lint for it)

erickt · 2015-04-03T16:07:50Z

@nikomatsakis: Yes, this PR explicitly handles #[derive(Copy, Clone)]. A lint sounds fine to me.

This extracts some of the minor cleanup patches from #23905.

huonw · 2015-04-06T04:55:14Z

Oh, I wasn't clear: my reading of the PR was that the support for that was only for staging reasons. In any case, a lint (or nothing) sounds good to me.

This extracts some of the minor cleanup patches from #23905.

dcrewi · 2015-04-13T13:36:54Z

This should probably be closed now, as the RFC has been resolved?

alexcrichton · 2015-04-13T17:04:38Z

Yes thanks for the reminder @dcrewi! Closing for the same reasons as listed in the RFC.

This extracts some of the minor cleanup patches from #23905.

rust-highfive assigned nikomatsakis Mar 31, 2015

sfackler reviewed Mar 31, 2015
View reviewed changes

erickt force-pushed the copy branch from bdf418d to a14f3a5Compare April 2, 2015 16:25

erickt added 8 commits April 2, 2015 16:05

syntax: Rename deriving/cmp/* to match their current names
77b21fa

syntax: Change deriving methods to take a &mut FnMut(P<Item>)
0dff891
This allows #[derive(...)]` to create more than one impl

test: PadOnLeft was never used
d1cb3b3

syntax: Clean up the indentation for #[derive(Eq)]
02c5ff7

syntax: Make #[derive(Copy)] imply #[derive(Clone)]
214113a

syntax: Change #[derive(Eq)] imply #[derive(PartialEq)]
f5b939a

syntax: Make #[derive(Ord)] imply #[derive(PartialOrd)]
6dbb0ef

erickt force-pushed the copy branch from a14f3a5 to eed443bCompare April 2, 2015 23:05

erickt mentioned this pull request Apr 2, 2015
Allow #[derive(...)] methods to generate multiple items, and other cleanup #23985
Merged

syntax: Fix the #[derive(Ord)] doc about the derived PartialOrd
6231cc0

bors added a commit that referenced this pull request Apr 6, 2015
Auto merge of#23985- erickt:derive-cleanup, r=huonw
7907099
This extracts some of the minor cleanup patches from #23905.

bors added a commit that referenced this pull request Apr 7, 2015
Auto merge of#23985- erickt:derive-cleanup, r=huonw
b44acd3
This extracts some of the minor cleanup patches from #23905.

This was referenced Apr 8, 2015
Automatically derive {Clone,PartialEq,PartialOrd} when {Copy,Eq,Ord} are derived rust-lang/rfcs#1028
Closed
Can't derive Copy for structs containing arrays #24288
Closed

alexcrichton closed this Apr 13, 2015

bors added a commit that referenced this pull request Apr 18, 2015
Auto merge of#23985- erickt:derive-cleanup, r=erickt
1284be4
This extracts some of the minor cleanup patches from #23905.

Uh oh!

Uh oh!

Conversation

erickt commented Mar 31, 2015

Uh oh!

rust-highfive commented Mar 31, 2015

Uh oh!

sfacklerMar 31, 2015

Choose a reason for hiding this comment

Uh oh!

ericktMar 31, 2015

Choose a reason for hiding this comment

Uh oh!

aturon commented Mar 31, 2015

Uh oh!

alexcrichton commented Mar 31, 2015

Uh oh!

sfackler commented Mar 31, 2015

Uh oh!

alexcrichton commented Mar 31, 2015

Uh oh!

erickt commented Apr 1, 2015

Uh oh!

nikomatsakis commented Apr 1, 2015

Uh oh!

erickt commented Apr 1, 2015

Uh oh!

nikomatsakis commented Apr 1, 2015

Uh oh!

bors commented Apr 2, 2015

Uh oh!

erickt commented Apr 2, 2015

Uh oh!

huonw commented Apr 3, 2015

Uh oh!

nikomatsakis commented Apr 3, 2015

Uh oh!

erickt commented Apr 3, 2015

Uh oh!

huonw commented Apr 6, 2015

Uh oh!

dcrewi commented Apr 13, 2015

Uh oh!

alexcrichton commented Apr 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants