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

Equivalence trait instead of Borrow #183

Open
stepancheg opened this issue Feb 10, 2022 · 8 comments · May be fixed by #274
Open

Equivalence trait instead of Borrow #183

stepancheg opened this issue Feb 10, 2022 · 8 comments · May be fixed by #274
Labels
enhancement New feature or request help wanted Extra attention is needed p-low Low priority issue

Comments

@stepancheg
Copy link
Contributor

See how IndexMap is doing it: https://docs.rs/indexmap/latest/indexmap/map/struct.IndexMap.html#method.get

Equivalent trait is implemented automatically for Borrow, but additionally it can be implemented for complex types. Consider this example:

struct Key {
  k0: String,
  k1: String,
}

let map: DashMap<Key, u32> = ...

struct KeyRef<'a> {
  k0: &'a str,
  k1: &'a str,
}

let key_ref: KeyRef = ...

map.get(???)

With indexmap it is possible to implement KeyRef: Equivalent<Key> and query by KeyRef.

It is not possible to implement reasonable Borrow for Key (without allocating).
@xacrimon xacrimon added enhancement New feature or request help wanted Extra attention is needed p-high High priority issue labels Feb 21, 2022
@xacrimon
Copy link
Owner

Looked into this. It seems to be currently blocked by rust-lang/rust#56167 as we use the std hashmap under the hood.

@stepancheg stepancheg mentioned this issue Mar 24, 2022
Merged
@xacrimon
Copy link
Owner

Now unblocked as we've switched to hashbrown.

@xacrimon xacrimon added p-low Low priority issue and removed p-high High priority issue labels Apr 27, 2022
@tomkarw tomkarw linked a pull request Aug 9, 2023 that will close this issue
Open
@tomkarw
Copy link
Contributor

tomkarw commented Aug 9, 2023

@xacrimon This one's pretty straightforward. PR and merge?

@stepancheg
Copy link
Contributor Author

Common Equivalent now lives in a separate crate by the way: https://docs.rs/equivalent/latest/equivalent/

@tomkarw
Copy link
Contributor

tomkarw commented Aug 10, 2023

Common Equivalent now lives in a separate crate by the way: https://docs.rs/equivalent/latest/equivalent/

That's true, but we care about whatever Equivalent trait hashbrown uses. So I think it's correct that the reexport chain is equivalent -> hashbrown -> dashmap.

@stepancheg
Copy link
Contributor Author

we care about whatever Equivalent trait hashbrown uses

Users of indexmap may prefer to implement Equivalent for their types from equivalent trait once for all possible map implementations using equivalent, without dependency on hashbrown, or indexmap, or dashmap.

@tomkarw
Copy link
Contributor

tomkarw commented Aug 10, 2023

Users of indexmap may prefer to implement Equivalent for their types from equivalent trait once for all possible map implementations using equivalent, without dependency on hashbrown, or indexmap, or dashmap.

That's true, however, they are free to do that. Even if we reexport the trait as dashmap::Equivalent for convenience, they can depend on equivalent::Equivalent, implement that and have it work in dashmap as well as other implementations.

Just to be clear, is your preferred solution different from this?

@stepancheg
Copy link
Contributor Author

@tomkarw well, this crate exports hashbrown HashMap in public API, then you are correct, it should be hashbrown's Equivalent. I just learned it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed p-low Low priority issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: replace Borrow requirement on key with hashbrown::Equivalent tomkarw/dashmap
3 participants
@stepancheg @xacrimon @tomkarw