Skip to content

Add MemoryExtensions.CountAny/ReplaceAny#112951

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:svextensions
Feb 26, 2025
Merged

Add MemoryExtensions.CountAny/ReplaceAny#112951
stephentoub merged 2 commits into
dotnet:mainfrom
stephentoub:svextensions

Conversation

@stephentoub

Copy link
Copy Markdown
Member

Closes #109859

@stephentoub stephentoub added this to the 10.0.0 milestone Feb 26, 2025
Copilot AI review requested due to automatic review settings February 26, 2025 14:38
@ghost

ghost commented Feb 26, 2025

Copy link
Copy Markdown

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

ghost commented Feb 26, 2025

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR Overview

This pull request introduces new APIs for counting and replacing elements in spans by adding CountAny, ReplaceAny, and ReplaceAnyExcept methods. It also adds corresponding tests to validate the behavior of these new methods in both in-place and source-destination scenarios.

  • Added tests for ReadOnlySpan.CountAny and Span.ReplaceAny/ReplaceAnyExcept in System.Memory.
  • Implemented new overloads in MemoryExtensions.cs to support CountAny methods and source/destination based replace semantics.
  • Updated the reference assembly to include method signatures for the new APIs.

Reviewed Changes

File Description
src/libraries/System.Memory/tests/ReadOnlySpan/CountAny.cs Adds tests for CountAny extension methods on ReadOnlySpan.
src/libraries/System.Memory/tests/Span/ReplaceAny.cs Adds tests for in-place and source/destination overloads of ReplaceAny and ReplaceAnyExcept.
src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Implements the new CountAny, ReplaceAny, and ReplaceAnyExcept extension methods.
src/libraries/System.Memory/ref/System.Memory.cs Updates reference signatures for the newly added methods.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/CountAny.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@stephentoub

Copy link
Copy Markdown
Member Author

/ba-g leg timed out

@stephentoub stephentoub merged commit 2fef827 into dotnet:main Feb 26, 2025

@IDisposable IDisposable left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it worth checking for and preventing in .ReplaceAny an endless loop if the supplied newValue is contained in the values being replaced? Before starting the loop we could do a test like

if (values.Contains(newValue)) // throw something

For .ReplaceAnyExcept the test would be to ensure that the newValue is included in the values passed and throwing if not, so that we're not endlessly looping.

I don't know if this is the sort of thing we should even be putting in the library, but this is a preventable footgun, so maybe?

@MihaZupan

MihaZupan commented Feb 27, 2025

Copy link
Copy Markdown
Member

The span is always sliced at pos + 1, so you're guaranteed to make progress even if the element you've just replaced would have also been a match. You shouldn't ever get into an endless loop.

Doing something like

const string InvalidChars = "!#?_";
span.ReplaceAny(InvalidChars, '_');

is valid, even if it would behave the same even if the user dropped _ from the values set.

@IDisposable

Copy link
Copy Markdown
Contributor

RIght! I missed that, thanks.

@stephentoub stephentoub deleted the svextensions branch February 27, 2025 01:19
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 29, 2025
@ericstj

ericstj commented Apr 4, 2025

Copy link
Copy Markdown
Member

@MihaZupan @stephentoub do you think this API warrants a note in dotnet/core#9824?

@stephentoub

Copy link
Copy Markdown
Member Author

They're small but useful helpers.

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.

[API Proposal]: More MemoryExtension methods accepting SearchValues<T>

5 participants