StringSegment: more AsSpan overloads by gfoidl · Pull Request #53463 · dotnet/runtime

gfoidl · 2021-05-29T12:36:36Z

ghost · 2021-05-29T12:36:42Z

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, to 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 · 2021-05-29T12:36:44Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #50428

Author:	gfoidl
Assignees:	-
Labels:	`area-Extensions-Primitives`, `new-api-needs-documentation`
Milestone:	-

xtqqczze · 2021-06-01T02:55:07Z

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

+        /// </exception>
+        public ReadOnlySpan<char> AsSpan(int start, int length)
+        {
+            if (!HasValue || (start | length) < 0 || (uint)(start + length) > (uint)Length)


Suggested change

if (!HasValue || (start | length) < 0 || (uint)(start + length) > (uint)Length)

if (!HasValue || start < 0 || (uint)(start + length) > (uint)Length)

MemoryExtensions.AsSpan(this string text, int start, int length) throws when length is negative, therefore this check is redundant.

Good catch!

The difference is that MemoryExtensions.AsSpan throws AOR with paramName start only, whilst the check here distinguishes between start and length to be on par with SubSegment, and SubString.

So what to do 🤷‍♂️

OK, I missed that paramName was always start in MemoryExtensions.AsSpan. This seems like a bug?

Span(T[] array, int start, int length) just avoids specifying the paramName, which is maybe the best approach?

@eerhardt do you think this is a bug in MemoryExtensions.AsSpan?
Or who is the right person to ask this?

@GrabYourPitchforks is the person I'd ask...

It feels like a bug to me. It definitely doesn't seem correct that MemoryExtensions.AsSpan and [ReadOnly]Span.Slice act differently here. I assume the Span.Slice behavior is "more correct" because you can't definitively tell which parameter is really at fault here without doing more checks which just adds code to a very hot path.

Maybe the best approach would be to open an issue describing the difference. I don't think we should try to solve it here.

also FYI @tarekgh. Tracing back to dotnet/corefx@a68803c, it appears Span.Slice used start before that commit, but during that commit Span got split into Fast and Portable. The Portable path still used start, but the Fast path dropped the parameter name.

Filed #53622 for this.

The MemoryExtensions behavior is intentional, but I agree it's weird. We want the API to be as fast as possible, which means we don't have the luxury of figuring out which of start or length was incorrect. We could pass a nullptr argument name to the ArgumentOutOfRangeException ctor, but honestly nobody expects ArgumentException.ParamName to be nullptr. So always passing "start" seems like the best solution given these constraints. If it's really a problem in practice, we could always create a detailed error message, just for this one scenario.

We could pass a nullptr argument name to the ArgumentOutOfRangeException ctor, but honestly nobody expects ArgumentException.ParamName to be nullptr.

But it is OK for Span?

runtime/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

Lines 350 to 359 in dbb05eb

public ReadOnlySpan<T> Slice(int start, int length)

{

#if TARGET_64BIT

// See comment in Span<T>.Slice for how this works.

if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)

ThrowHelper.ThrowArgumentOutOfRangeException();

#else

if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))

ThrowHelper.ThrowArgumentOutOfRangeException();

#endif

(I guess we can have this discussion in #53622 😄)

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs

eerhardt

This looks good to me. Thanks for the contribution here @gfoidl!

GrabYourPitchforks · 2021-06-02T18:10:56Z

I'll be the problem child. :)

Nit: I think these parameters should be called offset and length to match the parameter names on the existing StringSegment.Subsegment and StringSegment.Substring instance methods. Symmetry with existing methods on the type is generally preferred over symmetry with the MemoryExtensions.AsSpan extension methods.

@bartonjs @terrajobst @eerhardt thoughts?

bartonjs · 2021-06-02T18:16:00Z

I think these parameters should be called offset and length

My recollection is that this came up in the meeting, and we decided that "AsSpan"-consistency was the way to go.

string.Substring is startIndex/length; string.AsSpan is start/length. So AsSpan-consistency matches what we did for String.

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

This reverts commit da1027e.

eerhardt · 2021-06-02T23:29:28Z

@GrabYourPitchforks - I'm satisfied with this PR and think it is ready to merge. Let me know if you have any objections.

GrabYourPitchforks · 2021-06-03T00:50:07Z

@eerhardt Go for it!

gfoidl added 4 commits May 28, 2021 17:58

Defined API

b27205c

Added tests

faed7a1

Implementation

1a76e22

Remove some branches

da1027e

ghost added the area-Extensions-Primitives label May 29, 2021

ghost added the new-api-needs-documentation label May 29, 2021

phocodes approved these changes May 29, 2021

View reviewed changes

xtqqczze reviewed Jun 1, 2021

View reviewed changes

eerhardt reviewed Jun 1, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 1, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 1, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs Show resolved Hide resolved

PR Feedback

4e4a766

eerhardt approved these changes Jun 2, 2021

View reviewed changes

gfoidl mentioned this pull request Jun 2, 2021

MemoryExtensions.AsSpan/AsMemory throws different exception than {ReadOnly}Span.Slice #53622

Open

GrabYourPitchforks reviewed Jun 2, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

gfoidl added 2 commits June 2, 2021 20:38

Revert "Remove some branches" (partially)

7c5bc69

This reverts commit da1027e.

Don't use default argument in ThrowInvalidArguments

2059a10

GrabYourPitchforks approved these changes Jun 3, 2021

View reviewed changes

eerhardt merged commit 253a253 into dotnet:main Jun 3, 2021

gfoidl deleted the stringsegment-asspan branch June 4, 2021 07:08

gfoidl mentioned this pull request Jun 4, 2021

StringSegment IndexOf behavior #53727

Closed

ghost locked as resolved and limited conversation to collaborators Jul 4, 2021

	if (!HasValue \|\| (start \| length) < 0 \|\| (uint)(start + length) > (uint)Length)
	if (!HasValue \|\| start < 0 \|\| (uint)(start + length) > (uint)Length)

	public ReadOnlySpan<T> Slice(int start, int length)
	{
	#if TARGET_64BIT
	// See comment in Span<T>.Slice for how this works.
	if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#else
	if ((uint)start > (uint)_length \|\| (uint)length > (uint)(_length - start))
	ThrowHelper.ThrowArgumentOutOfRangeException();
	#endif

Conversation

gfoidl commented May 29, 2021

Uh oh!

ghost commented May 29, 2021

Uh oh!

ghost commented May 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfoidl Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks commented Jun 2, 2021

Uh oh!

bartonjs commented Jun 2, 2021

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Jun 2, 2021

Uh oh!

GrabYourPitchforks commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gfoidl Jun 1, 2021 •

edited

Loading

eerhardt Jun 1, 2021 •

edited

Loading