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.

Sign up for GitHub

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

Jump to bottom

Include "simple" UTF-8 validation and transcoding logic for interpreted and low-footprint scenarios #49372

Merged
merged 9 commits into from
Mar 17, 2021
Merged

Include "simple" UTF-8 validation and transcoding logic for interpreted and low-footprint scenarios #49372

merged 9 commits into from
Mar 17, 2021

Conversation

pavelsavara
Copy link
Member

trying to run unit tests on
#48006

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dnfadmin
Copy link

dnfadmin commented Mar 9, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@GrabYourPitchforks
Copy link
Member

Left some minor comments, but the code and the approach generally LGTM. But I'd like to see further discussion on #48006 as to whether it's even worthwhile to take this, and if so on what platforms we should conditionally compile this in. The initial platform list proposed in the .csproj here seems quite reasonable.

@pavelsavara
Copy link
Member Author

pavelsavara commented Mar 10, 2021 •
edited
Loading

Hi guys,

I'm learning dotnet contributor ropes on this task, so please bear with me.
I'm still struggling with running it and debugging it locally, so this draft PR was just easy way for me to run unit tests on it and see what happens. So it was just blind shot yesterday and there were errors in the logic.

I will further improve it based on the feedback, thanks for it.

Regarding question, if this should be merged at all, I thought we could just try and see if the change makes it actually smaller.
I still don't know where to look for the binaries to compare, but eventually I would get there.

There is also Utf16Utility shall we consider those too ?
Remaining methods in Utf8Utility look small enough to me, right ?

Thanks!

@pavelsavara pavelsavara marked this pull request as ready for review March 10, 2021 20:55
@pavelsavara pavelsavara mentioned this pull request Mar 10, 2021
Closed
@pavelsavara pavelsavara changed the title WIP: Include "simple" UTF-8 validation and transcoding logic for interpreted and low-footprint scenarios Include "simple" UTF-8 validation and transcoding logic for interpreted and low-footprint scenarios Mar 12, 2021
GrabYourPitchforks
GrabYourPitchforks approved these changes Mar 12, 2021
View reviewed changes
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, and thanks for your contribution! I'll defer to other more knowledgeable people on whether the switched platforms are correct. (To those reviewers, check the .csproj.)

@pavelsavara
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 49372 in repo dotnet/runtime

@GrabYourPitchforks GrabYourPitchforks merged commit 197a28d into dotnet:main Mar 17, 2021
@SingleAccretion SingleAccretion mentioned this pull request Mar 30, 2021
Merged
@danmoseley danmoseley mentioned this pull request Apr 9, 2021
Closed
radekdoulik added a commit to radekdoulik/runtime that referenced this pull request Apr 16, 2021
@radekdoulik
Revert "Include "simple" UTF-8 validation and transcoding logic for i…
5f723d4 
…nterpreted and low-footprint scenarios (dotnet#49372)"

This reverts commit 197a28d.

Context: dotnet#50260
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2021
@pavelsavara pavelsavara deleted the pavelsavara/48006 branch April 26, 2021 09:19
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pavelsavara @dnfadmin @GrabYourPitchforks @jkotas @karelz