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

Implementing From<Arc<str>> for SmolStr and From<SmolStr> for Arc<str> #58

Merged
merged 2 commits into from
Sep 10, 2023

Conversation

msdrigg
Copy link
Contributor

@msdrigg msdrigg commented Aug 25, 2023

Solves #57

  • Adding implementations to convert SmolStr to and from Arc<str>
  • Also adding one test to verify

The From<SmolStr> implementation is good. It saves an alloc in the Repr::Heap case, and allocates in the stack cases. It should be good to go.

The From<Arc<str>> does currently does not check for inline-able or whitespace strings. All Arc<str> become Repr::Heap. If this is a problem, could I get some feedback on how to solve it? Currently only SmolStr::new contains the code to detect inlines and whitespaces, so there are two options

  • I could duplicate this whole code block into the From impl
  • Or I could split it off into a separate function like fn new_on_stack(text: &str) -> Option<SmolStr> and then use that in Smol::new and From<Arc<str>>

@Veykril
Copy link
Member

Veykril commented Sep 3, 2023

We should check the length for this and not use heap if small enough. I'd say let's just go with your second outlined option for that to reduce code duplication

@msdrigg
Copy link
Contributor Author

msdrigg commented Sep 5, 2023

Now it only goes on the stack if it can

@lnicola
Copy link
Member

lnicola commented Sep 10, 2023

r? @Veykril

src/lib.rs Show resolved Hide resolved
@Veykril Veykril merged commit 7d1b60c into rust-analyzer:master Sep 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants