Skip to content

QueryStringEnumerable now works in terms of ReadOnlyMemory<char> #34001

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

Merged
merged 2 commits into from
Jul 1, 2021

Conversation

SteveSandersonMS
Copy link
Member

While using the new QueryStringEnumerable to implement #33338, I realized it would be far more useful if the APIs were based around ReadOnlyMemory<char> instead of ReadOnlySpan<char>.

This doesn't incur any extra perf cost in realistic cases, because the querystring is basically always going to come from an actual System.String on the heap, so we can represent that as ReadOnlyMemory<char> without any further allocations.

The benefit is that, now:

  • It's no longer a ref struct, because it doesn't need to be. Feel free to do normal things with it.
  • The actual parsed values (e.g., EncodedName, DecodeName(), etc.) are no longer ref structs either, so you can do things like use them as keys or values in dictionaries, or store them on the heap for any other reason without having to allocate a new string.

I'm not aware of any drawbacks in realistic scenarios.

@SteveSandersonMS SteveSandersonMS added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 1, 2021
@ghost
Copy link

ghost commented Jul 1, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@SteveSandersonMS SteveSandersonMS merged commit 238f7dd into main Jul 1, 2021
@SteveSandersonMS SteveSandersonMS deleted the stevesa/QueryStringEnumeratorShouldBeMemory branch July 1, 2021 22:41
@ghost ghost added this to the 6.0-preview7 milestone Jul 1, 2021
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Jul 10, 2021
Microsoft.AspNetCore.WebUtilities.QueryStringEnumerable.EncodedNameValuePair.DecodeValue() -> System.ReadOnlySpan<char>
Microsoft.AspNetCore.WebUtilities.QueryStringEnumerable.EncodedNameValuePair.EncodedName.get -> System.ReadOnlySpan<char>
Microsoft.AspNetCore.WebUtilities.QueryStringEnumerable.EncodedNameValuePair.EncodedValue.get -> System.ReadOnlySpan<char>
Microsoft.AspNetCore.WebUtilities.QueryStringEnumerable.EncodedNameValuePair.DecodeName() -> System.ReadOnlyMemory<char>
Copy link
Contributor

Choose a reason for hiding this comment

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

@halter73 to follow up with the .NET API review folks to figure out if returning ReadOnlyMemory is acceptable. API tentatively approved by ASP.NET Core review.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants