dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.29k stars 4.74k forks source link

[API Proposal]: StringBuilder.Insert(int, StringBuilder) #65519

Open jhudsoncedaron opened 2 years ago

jhudsoncedaron commented 2 years ago

Background and motivation

Had this appear where we are doing some really complex string building. There's no good way to feed one string builder into another, and partwise-insertion of strings with StringBuilder.Insert() results in way too many calls to MakeRoom and a very fragmented StringBuilder.

Analysis turned up a simple algorithm using two levels of StringBuilder; but it has extra copies because there is no StringBuilder.Insert(int, StringBuilder)

API Proposal

    public partial class StringBuilder {
        StringBuilder Insert(int index, StringBuilder? value);
    }

Logically this would do StringBuilder.Insert(int index, StringBuilder? value) => Insert(index, value?.ToString()); but running inside StringBuilder itself there would be two fewer copies.

API Usage

    StringBuilder s1 = new();
    AppendHeader(s1);
    int index = s1.Length;
    HashMap<Construct> components = new();
    s1.Append(" WHERE ");
    RecursiveBuildup(s1, components);
    StringBuilder s2 = new();
    foreach (var component in components.OrderBy((x) => x))
        s2.Append(" JOIN ").Append(component .Table).Append(" ON ").Append(component .LeftSide).Append(" = ").Append(component.RightSide);
    s1.Insert(index, s2); // Here's the new API being used
    s1.Append(';');

Alternative Designs

I have implemented a StringBuilder-like thing that builds up in a single array so that it can use StringBuilder.Insert(int, ReadOnlySpan<char>)

Risks

None

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

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

Issue Details
### Background and motivation Had this appear where we are doing some really complex string building. There's no good way to feed one string builder into another, and partwise-insertion of strings with StringBuilder.Insert() results in way too many calls to MakeRoom and a very fragmented StringBuilder. Analysis turned up a simple algorithm using two levels of StringBuilder; but it has extra copies because there is no `StringBuilder.Insert(int, StringBuilder)` ### API Proposal ```` public partial class StringBuilder { StringBuilder Insert(int index, StringBuilder? value); StringBuilder Append(StringBuilder? value); // For symmetry } ```` Logically this would do `StringBuilder.Insert(int index, StringBuilder? value) => Insert(index, value?.ToString())`; but running inside StringBuilder itself there would be two fewer copies. ### API Usage ```` StringBuilder s1 = new(); AppendHeader(s1); int index = s1.Length; HashMap components = new(); s1.Append(" WHERE "); RecursiveBuildup(s1, components); StringBuilder s2 = new(); foreach (var component in components.OrderBy((x) => x)) s2.Append(" JOIN ").Append(component .Table).Append(" ON ").Append(component .LeftSide).Append(" = ").Append(component.RightSide); s1.Insert(index, s2); // Here's the new API being used s1.Append(';'); ```` ### Alternative Designs I have implemented a `StringBuilder`-like thing that builds up in a single array so that it can use `StringBuilder.Insert(int, ReadOnlySpan)` ### Risks None
Author: jhudsoncedaron
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`, `untriaged`
Milestone: -
stephentoub commented 2 years ago

StringBuilder Append(StringBuilder? value); // For symmetry

That already exists: https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.append?view=net-6.0#system-text-stringbuilder-append(system-text-stringbuilder)

jhudsoncedaron commented 2 years ago

I will now investigate why I didn't find it. Ah. Only .Append(StringBuilder) exists, not .Insert(int, Stringbuilder).

tannergooding commented 2 years ago

@stephentoub, do you think this is worth exposing? If so I'll mark as api-ready-for-review

stephentoub commented 2 years ago

I don't have a strong opinion about it. We added Append(StringBuilder), so if we believe there were scenarios for that, I can see wanting an Insert(int, StringBuilder) as well.

stephentoub commented 2 years ago

(Are there other argument types for which we have either Append or Insert but not the other?)

jhudsoncedaron commented 2 years ago

I didn't look. I only feed string, char, char[], Span<char>, ReadOnlySpan<char>, or another StringBuilder to StringBuilder. Everything else has to be converted via IFormatProvider first.

There is an AppendFormat but no InsertFormat though. I haven't reached the optimization threshold where I have to tighten .Append(3.ToString(CultureInfo.something)) to .AppendFormat yet.