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

Fix Borrow #2433

Closed
SoniEx2 opened this issue May 7, 2018 · 13 comments
Closed

Fix Borrow #2433

SoniEx2 opened this issue May 7, 2018 · 13 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@SoniEx2
Copy link

SoniEx2 commented May 7, 2018

std::borrow::Borrow has an extremely limited API, making the following painful:

struct Thing (u8, String);
let m: HashMap<Thing, String> = Default::default();
// how to access m with u8, &str, avoiding allocations?

I recommend changing Borrow into Borrow<'a, T> so you can do Borrow<'a, &'a str> for String and derive it for multifield structs and stuff, reducing allocations all over the place.

@steveklabnik
Copy link
Member

steveklabnik commented May 7, 2018 via email

@SoniEx2
Copy link
Author

SoniEx2 commented May 7, 2018

No? It just lets you have (u8, String) borrowing into (&u8, &str), so that both can be used to index a HashMap. It would also support Cow<(&u8, &str)> if you wanted it to.

@SoniEx2
Copy link
Author

SoniEx2 commented May 7, 2018

Alternative: Key trait, with impl Key for T where T: Borrow.

@burdges
Copy link

burdges commented May 7, 2018

See the long thread on improving the Entry API via a new Query type.

@SoniEx2
Copy link
Author

SoniEx2 commented May 7, 2018

Key would be like, Key<'a, T>.

HashMap would take where K: Key<Q>, just like Borrow but different

(also, link?)

@SimonSapin
Copy link
Contributor

The Borrow trait is #[stable], so making breaking changes to it such as adding a lifetime parameter is not acceptable.

I agree that HashMap with keys that contain String is missing some way to do a lookup based on &str, but fixing that requires at least a formal RFC with much more details and discussion of alternatives and trade-offs (probably after some exploratory discussion on internals.rlo), not just filing a two-sentences issue.

@SoniEx2
Copy link
Author

SoniEx2 commented May 8, 2018

surely we can change this in rust 2.0?

the Key<'a, T> solution would work, and be backwards-compatible. (anything currently assuming it takes a Borrow would still work because Key would be automatically provided for Borrow impls)

@Centril
Copy link
Contributor

Centril commented May 8, 2018

surely we can change this in rust 2.0?

There will never be a Rust 2.0 afaik.

@SimonSapin
Copy link
Contributor

the Key<'a, T> solution would work, and be backwards-compatible

Then consider writing a much more detailed proposal for what that solution is

@burdges
Copy link

burdges commented May 8, 2018

Afaik, all existing work towards doing this correctly lives in #1769 and https://internals.rust-lang.org/t/pre-rfc-abandonning-morals-in-the-name-of-performance-the-raw-entry-api/7043 so this can probably be closed in favor of continuing those discussions somewhere like internals.

@SoniEx2
Copy link
Author

SoniEx2 commented May 8, 2018

hmm nope, that isn't what I have/had in mind at all.

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 9, 2018
@MajorBreakfast
Copy link
Contributor

The best way to discuss a proposal without writing a full RFC is by opening a pre-RFC thread in the internals forum (https://internals.rust-lang.org/). Just try it: You'll quickly notice that most Rust users who respond to github issues also read and post in the internals forum.

Lately, various users have expressed their concern that new features that are added to the language should be vetted more throughly [1] [2]. This makes it even more important to follow the proper procedure for changes to the languages. Making a change without a proper RFC is impossible. But, as already mentioned, you can always create a pre-RFC in the internals forum, work out the details and then write the full RFC that follows the RFC template.

@Centril
Copy link
Contributor

Centril commented May 18, 2018

Since this doesn't seem to be going anywhere, I'm closing this. See @MajorBreakfast's comment if you want to move forward with this.

@Centril Centril closed this as completed May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

6 participants