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

Move into std #3

Open
jplatte opened this issue Aug 21, 2023 · 6 comments
Open

Move into std #3

jplatte opened this issue Aug 21, 2023 · 6 comments

Comments

@jplatte
Copy link

jplatte commented Aug 21, 2023

I think that there could be problems w.r.t. inference, but has moving the traits from this crate into std been considered? I couldn't find any issues on the main rust repo about it.

@cuviper
Copy link
Member

cuviper commented Aug 21, 2023

I think that there could be problems w.r.t. inference,

FWIW, I noted that in indexmap-rs/indexmap#253 (comment), it actually seems to work better if K and Q are reversed, but I ended up not pursuing that further, in sort of a "worse is better" moment to just get something done. If experience shows that it really is better that way, that could be equivalent v2, or maybe a change to make when importing the idea to the standard library.

but has moving the traits from this crate into std been considered? I couldn't find any issues on the main rust repo about it.

I have not, nor would I likely have time for that, but it may make sense to see if it gets any real community use as an external crate first. Of course there are also users who will ignore it until it's in the standard library. If you're interested, feel free to propose this yourself!

@jplatte
Copy link
Author

jplatte commented Aug 21, 2023

Yeah, it's a bit annoying having to swap the std HashMap for hashbrown::HashMap just to get this, but I might end up doing it. I will likely also not have time to seriously drive an effort of moving this stuff into std, was mostly curious whether I had missed an existing effort that I could follow.

Thanks!

@JakkuSakura
Copy link

JakkuSakura commented Feb 20, 2024

it actually seems to work better if K and Q are reversed

It would be easier to implement Equivalent if it's inversed
Current:

enum Key {
  K(u32, ExternalKey)
}
impl Equivalent<Key> for (u32, ExternalKey) {
    ..
}

Inversed:

enum Key {
  K(u32, ExternalKey)
}
impl Equivalent<(u32, ExternalKey)> for Key {
    ..
}

Update: disregard this. I forgot there is the orphan rule with relaxed clause.

@clarfonthey
Copy link

clarfonthey commented Aug 3, 2024

Just poking in here since this was pointed out by rust-lang/rust#27242 (comment)

As far as crates.io is concerned, only 8 crates directly depend on equivalent, which makes it seem like not many crates have run into a problem where Equivalent works but Borrow doesn't. It would be nice for folks to offer compelling examples of where this trait was useful for them (since I don't doubt they exist), since right now very little data is easily available on how many people are using this.

Like, I think it'd be nice to have this in the standard library, but there's not really a compelling set of data to say it's needed there.

@GrigorenkoPV
Copy link

GrigorenkoPV commented Aug 4, 2024

where Equivalent works but Borrow doesn't. It would be nice for folks to offer compelling examples of where this trait was useful for them

https://github.com/droundy/internment/blob/8a5feb354d023a7c69f3889e9e737099ec0e4ea7/src/boxedset.rs#L49-L63

I think this one can be rewritten using these two traits, but not with std's Borrow and how it is currently used in HashMap.

Essentially we have HashSet<Box> and we want to do a lookup using &str. This would be possible if we had HashSet, but we need a Box for address stability. Currently this uses the raw_entry API, but I think it would be possible to rewrite it using something like

struct Newtype<T>(Box<T>);

impl<Q, K> Equivalent<K> for Newtype<Q>
where
    Q: Eq + ?Sized,
    K: Borrow<Q> + ?Sized,
{ ... }

type BoxedSet<T> = HashSet<NewType<T>>;

UPD: No, it's not possible. You have to do Equivalent<Newtype<Q>> for K, but the orphan rule won't let you. So yes,

it actually seems to work better if K and Q are reversed,

@al8n
Copy link

al8n commented Oct 12, 2024

I also want this crate to be moved into std, not only for HashMap, but also the Comparable trait for BTreeMap, the traits in this crate improve the flexibility of lookup comparing the current Borrow trait.

I opened a PR which uses Comparable trait instead of Borrow in crossbeam-skiplist, the flexibility given by the Comparable trait shows that around 10% performance improvement for the lookup.

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

No branches or pull requests

6 participants