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

Add some forwarding impls Hash, Ord, PartialOrd #37

Merged
merged 6 commits into from
Oct 27, 2022
Merged

Conversation

udoprog
Copy link
Owner

@udoprog udoprog commented Oct 27, 2022

I've omitted support for any variants in this PR, since they are a lot of work. But I've marked the relevant test cases with a TODO for now and we'll make sure that they're added before the next release (I only need unit enums right now for my downstream project).

I've also modified more existing trait impls to impl and forward to their underlying implementations. This is important to improve the chances that default trait functions also can be optimized, such as PartialEq::ne, or a != b.

The latter ought to be done for as much as possible.

@udoprog udoprog added the enhancement New feature or request label Oct 27, 2022
@udoprog udoprog requested a review from pitaj October 27, 2022 02:12
@udoprog
Copy link
Owner Author

udoprog commented Oct 27, 2022

I think the current sort order for unit enums isn't intuitive, or maybe incorrectly implemented.

See how the [a, b] example sorts to [b, a].

The reason we're getting this is because the comparison is performed over the storage impl Option<K> (or Option<()>):

#[derive(PartialEq, PartialOrd)]
enum Foo {
    First,
    Second,
}

fn main() {
    assert!(Foo::First < Foo::Second);
    assert!([Some(Foo::First), None] > [None, Some(Foo::Second)]);
}

@udoprog
Copy link
Owner Author

udoprog commented Oct 27, 2022

I'm very much considering whether or not to revert d85726c. It achieves the intuitive sort ordering, but through a much slower implementation.

The same result could be achieved by using a custom Option<T> implementation, but we'll have to check if it optimizes as well as std::option::Option before adopting it (and since Some and None are marked as lang items that might be hard fetched):

#[derive(PartialEq, PartialOrd)]
enum Option<T> {
    Some(T),
    None,
}

#[derive(PartialEq, PartialOrd)]
enum Foo {
    First,
    Second,
}

fn main() {
    assert!(Foo::First < Foo::Second);
    assert!([Option::Some(Foo::First), Option::None] < [Option::None, Option::Some(Foo::Second)]);
}

Playground.

The only difference between Opt<T> and std Option is the order in which the fields are defined, which ensures that a present value has higher priority than an absent one. Compare with https://doc.rust-lang.org/std/option/enum.Option.html.

@pitaj
Copy link
Collaborator

pitaj commented Oct 27, 2022

I don't think you need the forwarding for PartialEq::ne. They recently removed it from the PartialEq derive in rustc.

rust-lang/rust#98655

@udoprog udoprog merged commit b12add9 into main Oct 27, 2022
@udoprog udoprog deleted the more-forwarding branch October 27, 2022 17:17
names.push(name.clone());
entry.push(quote!(option_to_entry(#name, key)));
variants.push(&variant.ident);
names.push(format_ident!("_{}", index));
Copy link
Collaborator

@pitaj pitaj Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename variants and names to just variant and name?
Personally, I prefer #ident::#variant to #ident::#variants.
I feel like plurals should be reserved for cases where it's meant to be expanded directly into a list, like #(#field_inits,)*.

I think a better name than #ident would be great too. Maybe #target?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I don't have particularly strongly held opinions here. The larger improvements is reducing the number of vectors in use and moving the code closer to how it's used which makes it easier to work with.

One thought I have though is that pluralized names is useful to identify what's being looped over in the quote! macro call even though it looks a little awkward.

@udoprog
Copy link
Owner Author

udoprog commented Oct 27, 2022

I don't think you need the forwarding for PartialEq::ne. They recently removed it from the PartialEq derive in rustc.

rust-lang/rust#98655

Interesting! I suppose it does make sense that the codegen bloat to accomplish the same computational complexity isn't a worthwhile tradeoff (for when we generate an ne impl). And forwarding the impl is pretty much pointless but harmless. I'd be OK with removing those based on the information from that discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants