Slightly improve InternedString #5287
Conversation
|
✌️ @Eh2406 can now approve this pull request |
src/cargo/core/interning.rs
Outdated
| ptr: s.as_ptr(), | ||
| len: s.len(), | ||
| } | ||
| let s = match cache.get(str).map(|&s| s) { |
There was a problem hiding this comment.
Would this be clearer as unwrap_or_else?
There was a problem hiding this comment.
Awesome observation @Eh2406, updated!
I've actually spent about ten minutes trying to figure out why on earth cach.entry(str) does not work, only to find out that entry API for sets does not exist yet: rust-lang/rfcs#1490 :-)
Using unwrap_or_else didn't occur to me at all!
|
Thanks for doing this cleanup, and for documenting the problem with hash. I am sorry I did not add a comment to that effect after #5153 (comment). I wish we could separate hash for |
Interesting idea! Note that we have to do some funny business with hashes of |
For posterity, another potential interaction is that if we ever implement |
Or a serde serializer that returns the hash of the json representation or something.
Do you want to add a comment to that effect as well? |
Not really: already afk and probably we’ll think about it anyway when/if implementing Borrow :-) I’ve actually did add the Borrow impl at first, but then reasoned that WAGNI |
src/cargo/core/interning.rs
Outdated
| fn identity(s: &InternedString) -> (*const u8, usize) { | ||
| (s.inner.as_ptr(), s.inner.len()) | ||
| } | ||
| identity(self) == identity(other) |
There was a problem hiding this comment.
ptr::eq(self.inner, other.inner)?
There was a problem hiding this comment.
Wow, I haven't realized that *const str is a thing!
There was a problem hiding this comment.
@matklad I am willing to approve when you are happy. Do you want to make this change, or should I say the magick spell?
* Use `&'static str` instead of (ptr, len) pair to reduce unsafety. * try make hash calculation O(1) instead of O(n), fail miserably, document findings. * Rename `to_inner` -> `as_str()`.
|
@bors r+ |
|
📌 Commit d9880c3 has been approved by |
Slightly improve InternedString * Use `&'static str` instead of (ptr, len) pair to reduce unsafety. * try make hash calculation O(1) instead of O(n), fail miserably, document findings. * Rename `to_inner` -> `as_str()`.
|
☀️ Test successful - status-appveyor, status-travis |
&'static strinstead of (ptr, len) pair to reduce unsafety.document findings.
to_inner->as_str().