-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(atoms): supports static atoms #6812
Conversation
@@ -190,9 +219,9 @@ impl Ord for Atom { | |||
impl PartialEq for Atom { | |||
fn eq(&self, other: &Self) -> bool { | |||
// Fast path |
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.
@kdy1 After supporting Atom::static
, I'm not sure the fast path will work as before. To keep the logic same as before, we need to call match
twice. I'm going to remove the code, in case I miss something, what are your opinions?
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.
Also, should Atom
promise assert!(Atom::Arc("foo") == Atom::Static("foo"))
?
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 think it's fine, but assert!(Atom::Arc("foo") == Atom::Static("foo"))
should work
} | ||
|
||
// fn _assert_size() { | ||
// let _static_assert_size_eq = std::mem::transmute::<Atom, usize>; |
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.
@kdy1 The transmute
seems not to work, after making Atom(ThinArc<HeaderWithLength<()>, u8>)
to Atom(Inner)
.
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.
Yes, but it will degrade performance greatly.
I didn't implement static support because it's hard
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 get the gist. We need something like ThinStaticStr
. It's indeed hard after looking into thin_tr. I will try it, but it will take a while.
If anyone wants to try this, feel free to do it.
Description:
This PR supports allowing
Atom
to store&'static str
in it.BREAKING CHANGE:
Related issue (if exists):
#4946