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.

Sign up for GitHub

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

Jump to bottom

Deriving cmp::Ord #4269

Closed
Dretch opened this issue Dec 23, 2012 · 4 comments
Closed

Deriving cmp::Ord #4269

Dretch opened this issue Dec 23, 2012 · 4 comments
Assignees
@pcwalton
Labels
A-syntaxext Area: Syntax extensions A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@Dretch
Copy link
Contributor

Dretch commented Dec 23, 2012

It would be useful to be able to derive a cmp::Ord impl, similarly to how you can derive cmp::Eq impls.
@pcwalton
Copy link
Contributor

This is blocked on reforming Ord to use a Less, Equal, Greater enum like Haskell does.

@ghost ghost assigned pcwalton Dec 23, 2012
@ghost
Copy link

ghost commented Mar 10, 2013

I started work on deriving TotalOrd and have a preliminary version that supports #[deriving_total_ord] for generating an impl based on all of a struct's fields in the order they're defined as well as [#deriving_total_ord(field1, field2, ...)] for restricting/re-ordering the fields used to perform the comparison.

I'll attempt to get a pull request ready in the next few days.

@huonw
Copy link
Member

huonw commented Mar 29, 2013

As per #5588, I'm making good progress on this.

@huonw huonw mentioned this issue Mar 31, 2013
Closed
huonw added a commit to huonw/rust that referenced this issue Apr 12, 2013
@huonw
libsyntax: derive Clone, Eq, TotalEq, Ord, TotalOrd with the new gene…
7906c55 
…ric deriving code.

Closes rust-lang#4269, rust-lang#5588 and rust-lang#5589.
bors added a commit that referenced this issue Apr 12, 2013
@bors
auto merge of #5640 : dbaupp/rust/syntax-generalise-deriving, r=thest…
2cb6974 
…inger

This refactors much of the ast generation required for `deriving` instances into a common interface, so that new instances only need to specify what they do with the actual data, rather than worry about naming function arguments and extracting fields from structs and enum. (This all happens in `generic.rs`. I've tried to make sure it was well commented and explained, since it's a little abstract at points, but I'm sure it's still a little confusing.)

It makes instances like the comparison traits and `Clone` short and easy to write.

Caveats:
- Not surprisingly, this slows the expansion pass (in some cases, dramatically, specifically deriving Ord or TotalOrd on enums with many variants).   However, this shouldn't be too concerning, since in a more realistic case (compiling `core.rc`) the time increased by 0.01s, which isn't worth mentioning. And, it possibly slows type checking very slightly (about 2% worst case), but I'm having trouble measuring it (and I don't understand why this would happen). I think this could be resolved by using traits and encoding it all in the type system so that monomorphisation handles everything, but that would probably be a little tricky to arrange nicely, reduce flexibility and make compiling rustc take longer. (Maybe some judicious use of `#[inline(always)]` would help too; I'll have a bit of a play with it.)
- The abstraction is not currently powerful enough for:
  - `IterBytes`: doesn't support arguments of type other than `&Self`.
  - `Encodable`/`Decodable` (#5090): doesn't support traits with parameters.
  - `Rand` & `FromStr`; doesn't support static functions and arguments of type other than `&Self`.
   - `ToStr`: I don't think it supports returning `~str` yet, but I haven't actually tried.

  (The last 3 are traits that might be nice to have: the derived `ToStr`/`FromStr` could just read/write the same format as `fmt!("%?", x)`, like `Show` and `Read` in Haskell.)
 
  I have ideas to resolve all of these, but I feel like it would essentially be a simpler version of the `mt` & `ty_` parts of `ast.rs`, and I'm not sure if the simplification is worth having 2 copies of similar code.

Also, makes Ord, TotalOrd and TotalEq derivable (closes #4269, #5588 and #5589), although a snapshot is required before they can be used in the rust repo.

If there is anything that is unclear (or incorrect) either here or in the code, I'd like to get it pointed out now, so I can explain/fix it while I'm still intimately familiar with the code.
@thestinger
Copy link
Contributor

Fixed by #5640.

This was referenced Apr 13, 2013
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pcwalton pcwalton

Labels
A-syntaxext Area: Syntax extensions A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@pcwalton @huonw @Dretch @thestinger