Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Underlying CoreCLR support for new string slicing overloads - #15811

Merged
2 commits merged into
masterfrom
unknown repository
Jan 10, 2018
Merged

2 commits merged into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Jan 10, 2018

Copy link
Copy Markdown

https://github.com/dotnet/corefx/issues/25254

These add the underlying support for the fast versions of
these extension methods.

https://github.com/dotnet/corefx/issues/25254

These add the underlying support for the fast versions of
these extension methods.
@ghost ghost added the area-System.Memory label Jan 10, 2018
@ghost ghost self-assigned this Jan 10, 2018
@ghost
ghost requested a review from ahsonkhan January 10, 2018 16:44
@ahsonkhan

Copy link
Copy Markdown

This PR is part of https://github.com/dotnet/corefx/issues/24072, not 25254.


/// <summary>Creates a new <see cref="ReadOnlyMemory{T}"/> over the portion of the target string.</summary>
/// <param name="text">The target string.</param>
/// <exception cref="System.ArgumentNullException">Thrown when <paramref name="text"/> is a null reference (Nothing in Visual Basic).</exception>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it necessary to specify "Nothing in Visual Basic". If so, should we add that to other comments as well (for example: https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/MemoryExtensions.Portable.cs#L63). Is there a way to indicate that Nothing is a keyword?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The phrase appears in various XML docs (probably all copy-pasted from one instance as these were.) I don't really have a dog in this either way - I'd rather that this PR not be used as the place to debate that issue, though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fair enough.

/// <param name="text">The target string.</param>
/// <exception cref="System.ArgumentNullException">Thrown when <paramref name="text"/> is a null reference (Nothing in Visual Basic).</exception>
/// <exception cref="System.ArgumentOutOfRangeException">
/// Thrown when the specified <paramref name="start"/> index is not in range (&lt;0 or &gt;=text.Length).

@ahsonkhan ahsonkhan Jan 10, 2018 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment vs code mismatch. index is not in range (<0 or >=text.Length)

The code throws if start > text.Length

Here and elsewhere.

return new ReadOnlyMemory<char>(text, start, text.Length - start);
}

/// <summary>Creates a new <see cref="ReadOnlyMemory{T}"/> over the portion of the target string.</summary>

@ahsonkhan ahsonkhan Jan 10, 2018 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We create a ReadOnlyMemory<char> not ReadOnlyMemory{T}.

Here and elsewhere.

@ahsonkhan ahsonkhan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Otherwise, LGTM.

https://github.com/dotnet/corefx/issues/24072

These add the underlying support for the fast versions of
these extension methods.
@ghost
ghost merged commit da46977 into dotnet:master Jan 10, 2018
@ghost
ghost deleted the slice branch January 10, 2018 20:33
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants