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

[stdlib] Use UInt8 to store characters in String #2612

Closed

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented May 10, 2024

The fact that there are so many changes to do means that many functions access String's internal buffer without going through the method provided (for example unsafe_ptr()). It might be worth it to rework a bit the API so that when we change the String's internals later on, we don't have to touch too many things. I'm notably thinking here about SSO and Unicode.

This PR is part of #2317

Thanks god ASCII chars are between 0 and 127...

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@gabrieldemarmiesse gabrieldemarmiesse requested a review from a team as a code owner May 10, 2024 17:17
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

Nice!

stdlib/src/builtin/string.mojo Show resolved Hide resolved
@JoeLoser JoeLoser added the imported-internally Signals that a given pull request has been imported internally. label May 10, 2024
@gabrieldemarmiesse
Copy link
Contributor Author

Happy to see that 4433f95 was merged! I think we can close this PR now

@JoeLoser
Copy link
Collaborator

Happy to see that 4433f95 was merged! I think we can close this PR now

Yep, thanks for closing this out. As I mentioned at #2509 (comment), a lot of this stuff was intertwined internally, so it was easier for @rparolin to just make a bigger commit to knock it all out. Thanks for pushing on this!

@gabrieldemarmiesse gabrieldemarmiesse deleted the hello-world branch May 20, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants