-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use CompactStr #9229
Use CompactStr #9229
Conversation
@@ -844,7 +844,7 @@ where | |||
range: _, | |||
}) => { | |||
if let Expr::Name(ast::ExprName { id, ctx, range: _ }) = func.as_ref() { | |||
if id == "locals" && ctx.is_load() { | |||
if *id == "locals" && ctx.is_load() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this annoying. Not sure why &CompactStr == &'static str
doesn't work.
@BurntSushi probably knows why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure off the top of my head, so let's run through it. x == y
is syntactic sugar for std::cmp::PartialEq::eq(&x, &y)
. If id
is a &CompactString
and y
is a &'static str
, then &x
is actually an &&CompactString
and y
is actually a &&str
. So I believe this means we need to look for a PartialEq
impl for &&CompactString
(notice the double &
there).
The only PartialEq
impl for CompactString
is impl<T: AsRef<str>> PartialEq<T> for CompactString
, which on its own does not fit here because we're looking for &&CompactString
.
At least, I think that's why. It's possible the compact-str
crate could add more impls to alleviate this. But maybe not.
Auto-deref also comes into the picture here. See:
33d596d
to
61f6908
Compare
61f6908
to
b60ac75
Compare
CodSpeed Performance ReportMerging #9229 will improve performances by 4.81%Comparing Summary
Benchmarks breakdown
|
|
I'd also be interested in seeing how (Sorry, I know there are a billion "small string" crates...) |
I can try it later today |
Interesting, the benchmarks look better. (We almost never clone strings so perhaps O(1) clone is not an important tradeoff right now.) |
Hipstr has interesting properties but it might not work for us without dropping wasm support (playground):
Wasm is 32 bit if I remember it correctly. |
Yeah but the pointers are 32-bit too right? So that should be fine. I think the note in hipstr is talking about platforms with tagged pointers (like CHERI), which are pretty obscure at present. |
The problem with |
We'll revisit after merging the hand-written parser. |
Summary
To get a comparison to
SmolStr
which seems to optimize forO(1)
cloneI found the API a bit clumsy but maybe it's just me being confused by
Deref
.Test Plan
cargo test