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

[Feature Request] Ability to reserve String capacity directly #3738

Closed
1 task done
thatstoasty opened this issue Nov 4, 2024 · 7 comments
Closed
1 task done

[Feature Request] Ability to reserve String capacity directly #3738

thatstoasty opened this issue Nov 4, 2024 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library

Comments

@thatstoasty
Copy link
Contributor

Review Mojo's priorities

What is your request?

As the title says, I'd like to see what the community thinks about introducing a public API to allocate String capacity ala something like my_string.reserve(new_capacity=1000)?

What is your motivation for this change?

I had a use case where I was treating a String as a buffer so I could write bytes to it, where I roughly knew what the capacity would be ahead of time. Maybe having a StringBuilder struct would be better than using String for this purpose?

This is a continuation of the conversation started on this PR: #3735 (comment)

Any other details?

No response

@thatstoasty thatstoasty added enhancement New feature or request mojo-repo Tag all issues with this label labels Nov 4, 2024
@JoeLoser JoeLoser added good first issue Good for newcomers help wanted Extra attention is needed mojo-stdlib Tag for issues related to standard library labels Nov 5, 2024 — with Linear
@martinvuyk
Copy link
Contributor

martinvuyk commented Nov 5, 2024

String.reserve() would be super useful before concatenating a lot of Stringlike/AsBytes instances to a String, it would avoid much overhead from the __iadd__ dunders and from doing "".join(List[String]()) which would have required all items to be allocated as a string into the list, and then doing the whole concatenation.

Also I think it will be fitting to add a String(capacity=1_000) constructor.

I think we also need to implement Writer trait for List. That trait would also be useful in Span constrained to mutable Span[Byte] but that will probably be tricky given the current clunkyness of parametrized Origins.

Maybe having a StringBuilder struct would be better than using String for this purpose?

I'd rather reduce the amount of head-space people need to use the stdlib

@thatstoasty
Copy link
Contributor Author

@martinvuyk Yep, I share the exact same thoughts!

modularbot pushed a commit that referenced this issue Nov 9, 2024
…to the String struct (#50557)

[External] [stdlib] Addition of a new constructor and reserve method to
the String struct

Adds a public API for users to allocate capacity through String, instead
of having to reach into the private internal `._buffer` attribute.

Satisfies: #3738

Co-authored-by: Mikhail Tavarez <[email protected]>
Closes #3755
MODULAR_ORIG_COMMIT_REV_ID: 3d3c3c4fd49acaf3f8f204e453039bacfe57a8ac
@thatstoasty
Copy link
Contributor Author

This change was merged into the nightly branch, closing the issue.

@JoeLoser
Copy link
Collaborator

JoeLoser commented Nov 9, 2024

This change was merged into the nightly branch, closing the issue.

Thank you! Looks like "satisfies" in 55cbae5 didn't close the issue. Typically I use "Fixes" or "Closes" which is respected by the GitHub automation.

@thatstoasty
Copy link
Contributor Author

Ah, I didn't know there was automation around that! I'll keep that in mind for the future 🙂

@JoeLoser
Copy link
Collaborator

JoeLoser commented Nov 9, 2024

Ah, I didn't know there was automation around that! I'll keep that in mind for the future 🙂

No worries. Yeah, see https://github.blog/news-insights/product-news/closing-issues-via-commit-messages/. Old blog post but hasn't changed much I suspect.

@JoeLoser
Copy link
Collaborator

JoeLoser commented Nov 9, 2024

There's a fair bit of cleanup work we could do now to leverage the new constructor throughout the stdlib. This would have the benefit of less reaching into private data members, which is always a plus.

modularbot pushed a commit that referenced this issue Dec 17, 2024
…to the String struct (#50557)

[External] [stdlib] Addition of a new constructor and reserve method to
the String struct

Adds a public API for users to allocate capacity through String, instead
of having to reach into the private internal `._buffer` attribute.

Satisfies: #3738

Co-authored-by: Mikhail Tavarez <[email protected]>
Closes #3755
MODULAR_ORIG_COMMIT_REV_ID: 3d3c3c4fd49acaf3f8f204e453039bacfe57a8ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library
Projects
None yet
Development

No branches or pull requests

3 participants