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

{U}Int128 support for Utf8Formatter/Parser #86357

Closed
wants to merge 8 commits into from

Conversation

hrrrrustic
Copy link
Contributor

@hrrrrustic hrrrrustic commented May 16, 2023

Close #73842

The implementation is almost copy-paste from {U}Int64 as same as other integer types doing for previous size types. I can try to reduce some duplication, but not sure if it's possible without any performance losses

Holding draft to see CI tests, fix missed type casts and possible typos

Blocked by #73842 (comment)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 16, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 16, 2023

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Close #73842

The implementation is almost copy-paste from {U}Int64 as same as other types doing for previous size types. I can try to reduce some duplication, but not sure if it's possible without any performance losses

Holding draft to see CI tests

Author: hrrrrustic
Assignees: -
Labels:

area-System.Memory, new-api-needs-documentation, community-contribution

Milestone: -

@stephentoub
Copy link
Member

@tannergooding, do we still want new methods on Utf8Formatter for UInt128 / Int128? With UTF8 support now baked into the types, Utf8Formatter is largely obsolete.

@tannergooding
Copy link
Member

do we still want new methods on Utf8Formatter for UInt128 / Int128? With UTF8 support now baked into the types, Utf8Formatter is largely obsolete.

I think its probably worth a timeboxed discussion in API review. UTF8Parser still serves a purpose in that it treats the first invalid character as "end of string" rather than "failure.

Having Int128 support in the parser, but not a mirror in the formatter then seems a bit odd. Not being able to more generally express the parsing behavior is also a bit odd/limiting.

I think the ideal scenario, in my mind, would be to not expose this support for UTF8Formatter or UTF8Parser and instead expose a new NumberStyles.AllowInvalidCharacterForEndOfString (but a better name). This would allow both UTF8Formatter and UTF8Parser to both be "effectively obsolete" and to also bring the support up to the UTF16 space so we have parity.

If we did decide to provide it on UTF8Parser, then I think it would be best to also provide it on UTF8Formatter for consistency.

@stephentoub
Copy link
Member

UTF8Parser still serves a purpose

Yup. I was referring specifically to Utf8Formatter. The only argument I see for adding to it at this point is when we add something to Utf8Parser and then want some kind of consistency between them even though we would discourage anyone from using the Utf8Formatter APIs. While I'm all for consistency, adding APIs we don't actually want folks to use isn't desirable.

I think the ideal scenario, in my mind, would be to not expose this support for UTF8Formatter or UTF8Parser and instead expose a new NumberStyles.AllowInvalidCharacterForEndOfString (but a better name)

I'm fine with that, too. Then both Utf8Formatter and Utf8Parser are entirely obsolete, whether we choose to mark them as such or not.

@tannergooding
Copy link
Member

tannergooding commented Jun 6, 2023

I've opened #87171 covering the proposed new functionality. Depending on API review we may want to either keep the PR as is -or- to pull what's been implemented so far into the general parsing logic.

@ghost ghost closed this Jul 6, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Utf8Formatter/Utf8Parser should have UInt128 and Int128 overloads
3 participants