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.

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

[API Proposal]: Consider Equals method(s) on DefaultInterpolatedStringHandler to avoid temporary string allocations #110427

Open
BrennanConroy opened this issue Dec 5, 2024 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner

Comments

@BrennanConroy
Copy link
Member

BrennanConroy commented Dec 5, 2024

Background and motivation

Was just modifying some code and noticed it was creating a string from a number and adding it to other strings in an if statement just to compare to another string and then throw the just created string away. Started updating the code to remove the number allocation by using string interpolation, and then realized the interpolated string is still being allocated just to do a comparison and then thrown away. It seems a bit silly when we could do span comparisons to avoid the string allocation.

If this API was added, Roslyn would also want to update to generate code that makes use of the API as most people aren't going to be using DefaultInterpolatedStringHandler explicitly.

API Proposal

namespace System.Runtime.CompilerServices;

public ref struct DefaultInterpolatedStringHandler
{
+    public bool Equals(ReadOnlySpan<char> other, StringComparison comparisonType);
-    internal void Clear();
+    public void Clear();
}

API Usage

var str1 = "somevalue";
var str2 = "some";
var val1 = 1;
if (str1 == $"{str2}{val1}")
{
}

Ideally Roslyn generated diff:

string text = "somevalue";
string value = "some";
int value2 = 1;
DefaultInterpolatedStringHandler defaultInterpolatedStringHandler = new DefaultInterpolatedStringHandler(0, 2);
defaultInterpolatedStringHandler.AppendFormatted(value);
defaultInterpolatedStringHandler.AppendFormatted(value2);
-bool flag = defaultInterpolatedStringHandler.ToStringAndClear() == text;
+bool flag = defaultInterpolatedStringHandler.Equals(text, StringComparison.InvariantCulture);
+defaultInterpolatedStringHandler.Clear();

Alternative Designs

public ref struct DefaultInterpolatedStringHandler
{
+    public bool EqualsAndClear(ReadOnlySpan<char> other, StringComparison comparisonType);
}

Risks

DefaultInterpolatedStringHandler uses an array pool internally and returns the array to the pool when the string is realized. Separating the usage of the interpolated string from the clearing makes the type more error prone, so the alternative design follows the current ToStringAndClear public method where it combines the action and clearing the array.

@BrennanConroy BrennanConroy added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 5, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 5, 2024
@huoyaoyuan huoyaoyuan added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 5, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@huoyaoyuan
Copy link
Member

Equals is just one operation that can reduce allocation. Exposing the formatted Span would benefit more scenarios.

@stephentoub
Copy link
Member

Equals is just one operation that can reduce allocation. Exposing the formatted Span would benefit more scenarios.

Yes, if we believe we have real need for this capability, we should reconsider #55390 instead.

@BrennanConroy
Copy link
Member Author

I probably should have put the alternative design as the primary one. Like #55390 mentions, exposing more generic methods/properties makes the type more error prone. We could argue that since its primary use is by the compiler where we should be writing correct code, then making it a little more error prone is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants