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

Bump::alloc_str #50

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Bump::alloc_str #50

merged 1 commit into from
Nov 21, 2019

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Nov 21, 2019

This is similar to allocating slices, but for string slices. Because
string slices are a strange beast and none of the other methods can be
used directly. It could be done at the caller side, but then the user
would need to use unsafe which might be unacceptable in business-logic
applications or crates (or be content with the needless check for utf8
which is already guaranteed to be valid).

This is similar to allocating slices, but for string slices. Because
string slices are a strange beast and none of the other methods can be
used *directly*. It could be done at the caller side, but then the user
would need to use `unsafe` which might be unacceptable in business-logic
applications or crates (or be content with the needless check for utf8
which is already guaranteed to be valid).
Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Would you like to add a alloc_str_with function in a follow up PR as well?

@fitzgen fitzgen merged commit 1ade235 into fitzgen:master Nov 21, 2019
@vorner vorner deleted the alloc_str branch November 22, 2019 09:05
@vorner
Copy link
Contributor Author

vorner commented Nov 22, 2019

Awesome! Would you like to add a alloc_str_with function in a follow up PR as well?

I'm not against it and I'd add it, but I'm not sure about its semantics/signature. I mean, all these slice with methods create one T at a time. I don't think anyone is able to create one u8 at a time to build a valid utf8 string. Creating one char at a time is also problematic (both inconvenient and assembling the representation & computation of lengths might be a bit challange). A closure returning &str is a bit strange and how about lifetimes… and as that likely doesn't live on the stack, what problem/use case would such method have?

But it's possible I'm missing something obvious so if you just tell me how it should look like, I'll be happy to implement it.

@fitzgen
Copy link
Owner

fitzgen commented Nov 22, 2019

Ah, that isn't quite what I was thinking, but upon reflection what I was thinking might not make sense either.

This was what I was originally thinking:

fn alloc_str_with<'a>(&self, f: impl FnOnce() -> &'a str) -> &mut str {
    ...
}

This doesn't make sense for a couple reasons:

  1. we don't know how much space to allocate until after we call the function, which defeats the purpose of the with variant
  2. returning a reference means that the function can't create the string, only return existing strings, again defeating the purpose of the with variant

We could solve (2) by accepting any IntoIterator<Item=char> return, and we could solve (1) by requiring that the string's length is given before hand. The latter solution is pretty unergonomic and opens questions of whether to trust that number and make the method unsafe, or not to trust to trust that length and make the method safe, and perform bounds checks.

Ultimately, I think building strings in the bump arena (as opposed to copying them in) is better served by bumpalo::collections::String, and I've convinced myself we don't want to have an alloc_str_with method.

Anyways, thanks again for this PR!

@vorner
Copy link
Contributor Author

vorner commented Nov 22, 2019

If there's no follow-up PR for this feature, would it be OK to have a release?

@fitzgen
Copy link
Owner

fitzgen commented Nov 22, 2019

I'll file a new issue discussing a new release, since it is going to be a breaking release, I want to double check all the ducks are in the right rows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants