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

Don't use StaticString.utf8Start unless there is a pointer rep. #1015

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

thomasvl
Copy link
Collaborator

The docs for StaticString.utf8Start say it should runtime abort for this, so
play it safe and handling when a StaticString doesn't have a ptr rep.

@thomasvl thomasvl requested a review from tbkka June 24, 2020 19:49
@thomasvl
Copy link
Collaborator Author

@tbkka - @allevato noticed this when we were looking at the two issues that came in, it seems like this is the right thing to do to ensure things always work.

No clue if it is related to the two issues…

The docs for StaticString.utf8Start say it should runtime abort for this, so
play it safe and handling when a StaticString doesn't have a ptr rep.
@thomasvl thomasvl force-pushed the staticstring_not_have_ptr_rep branch from 11d6c4a to a7a5c13 Compare June 24, 2020 21:25
@tbkka
Copy link
Collaborator

tbkka commented Jun 25, 2020

I'm fine with the approach proposed here for working around the current concern, but I suspect this code can be radically simplified. A lot of the complexity here was motivated by issues with the original UTF-16 Swift String implementation, but things are very different now. For example, we can likely just drop the utf8Buffer property entirely (and all of the tedious interning) if we just pre-compute and store the hash value so that we can rapidly compare with identifiers in JSON or TextFormat input. Careful benchmarking is certainly warranted, but it's entirely possible that a more straightforward approach built on the standard String type would actually be faster than this code.

@thomasvl
Copy link
Collaborator Author

Yea, the whole parsing/serialization pipeline likely could get a revalidation due to the String changes, there might be a few places things can be streamlined.

@thomasvl thomasvl merged commit 003e8f8 into apple:master Jun 25, 2020
@thomasvl thomasvl deleted the staticstring_not_have_ptr_rep branch June 25, 2020 13:20
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