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

std::slice::SliceConcatExt impls should rely on AsRef rather than Borrow #26780

Closed
SimonSapin opened this issue Jul 5, 2015 · 7 comments
Closed

Comments

@SimonSapin
Copy link
Contributor

HashMap assumes that Borrow implies compatible hashing, which may not be desirable: servo/string-cache#89

@frewsxcv
Copy link
Member

frewsxcv commented Jul 5, 2015

@bluss
Copy link
Member

bluss commented Jul 5, 2015

See discussion on #25162 why it was changed to Borrow

@steveklabnik
Copy link
Member

Yes, @aturon @alexcrichton , is this actually intended? Seems so

@steveklabnik
Copy link
Member

/cc @rust-lang/libs

@Gankra
Copy link
Contributor

Gankra commented Jul 6, 2015

To be clear: Almost the entire reason for Borrow to exist is to have "AsRef where Hash/Eq/Ord are expected to hold". (the rest of the reason is differing blanket impls in a world without specialization)

(not clear on what @SimonSapin meant with "may not be desirable")

@alexcrichton
Copy link
Member

Yes this was switched to Borrow from AsRef as @bluss pointed out due to coherence issues. Whether or not this is desirable we cannot switch to AsRef today, so I'm going to close this as working as intended.

@SimonSapin
Copy link
Contributor Author

In string-cache we have:

#[derive(PartialEq, Eq, Hash)]
struct Atom { magic: u64 }
impl Deref for Atom { Target = str; ... }

The whole point of this library is to make Atom::eq fast. While we’re at it, we make Atom::hash fast in the same way. (That is, we only compare/hash the u64 value, not the str’s bytes.) The rest of the library ensures that *atom_1 == *atom_2 if and only if atom_1.magic == atom_2.magic.

To make make Atom::hash fast, we make it not consistent with str::hash. Therefore, Atom should not implement Borrow<str>.

SliceConcatExt however does not rely on hashing. Being able to use it on atoms would be nice.

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