Override IndentedTextWriter APIs that write spans.#130586
Override IndentedTextWriter APIs that write spans.#130586teo-tsirpanis wants to merge 4 commits into
IndentedTextWriter APIs that write spans.#130586Conversation
Saves an array pool rent/return cycle and copying.
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
There was a problem hiding this comment.
Pull request overview
This PR updates System.CodeDom.Compiler.IndentedTextWriter to override the span-based TextWriter write APIs so span writes can be forwarded directly to the inner writer without the base TextWriter array-pool rent/copy path.
Changes:
- Add
IndentedTextWriter.Write(ReadOnlySpan<char>)andWriteLine(ReadOnlySpan<char>)overrides in CoreLib. - Add the corresponding members to the
System.Runtimereference assembly forIndentedTextWriter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/CodeDom/Compiler/IndentedTextWriter.cs | Adds span-based Write / WriteLine overrides to avoid the TextWriter pooling/copy path. |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates the public contract to include the new IndentedTextWriter override members. |
| public override void Write(char value) { } | ||
| public override void Write(char[]? buffer) { } | ||
| public override void Write(char[] buffer, int index, int count) { } | ||
| public override void Write(ReadOnlySpan<char> buffer) { } |
There was a problem hiding this comment.
I don't think overriding a method needs approval.
Fixes race conditions if a synchronous operation is queued to run on another thread.
8569c2b to
1f0b6d5
Compare
| public override void Write(char[]? buffer) { } | ||
| public override void Write(char[] buffer, int index, int count) { } | ||
| public override void Write(System.ReadOnlySpan<char> buffer) { } | ||
| public override void Write(double value) { } |
| public override void Write(ReadOnlySpan<char> buffer) | ||
| { | ||
| OutputTabs(); | ||
| _writer.Write(buffer); | ||
| } |
| @@ -8122,6 +8122,7 @@ public override void Write(bool value) { } | |||
| public override void Write(char value) { } | |||
| public override void Write(char[]? buffer) { } | |||
| public override void Write(char[] buffer, int index, int count) { } | |||
| public override void Write(System.ReadOnlySpan<char> buffer) { } | |||
| public override void Write(double value) { } | |||
| public override void Write(ReadOnlySpan<char> buffer) | ||
| { | ||
| OutputTabs(); | ||
| _writer.Write(buffer); | ||
| } |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "f13639544f05bb56cf6ebb63c910660ea8abf4e8",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "0e7b627c152a3c1885d1db498d81d01de55a3391",
"last_reviewed_commit": "f13639544f05bb56cf6ebb63c910660ea8abf4e8",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "0e7b627c152a3c1885d1db498d81d01de55a3391",
"last_recorded_worker_run_id": "29681936126",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "f13639544f05bb56cf6ebb63c910660ea8abf4e8",
"review_id": 4730543703
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: IndentedTextWriter derives from TextWriter but did not override the span-based Write(ReadOnlySpan<char>) and WriteLine(ReadOnlySpan<char>) methods. The inherited base implementations copy the span into a pooled char[] (an ArrayPool rent/return cycle plus a copy) before delegating. Since IndentedTextWriter wraps an inner TextWriter that natively supports spans, this indirection is pure overhead.
Approach: The PR adds the two missing overrides, each calling OutputTabs() and forwarding the span directly to the inner _writer, consistent with every other synchronous overload in the class. WriteLine(ReadOnlySpan<char>) also sets _tabsPending = true, matching the existing WriteLine pattern so subsequent writes are correctly re-indented. The reference assembly (System.Runtime.cs) is updated with the two new public overrides. Tests are extended: the parameterized Writes/WriteLines data sources gain span cases, the aggregate Writes_ProducesExpectedOutput test exercises both new APIs, and the test's TrackingTextWriter now overrides the span methods to assert delegation. The async test overrides were also refactored from manual Task result = ...; return result; to async/await, and the previously-tracked WriteAsync(string)/WriteLineAsync(string) overrides were joined by ReadOnlyMemory<char> overloads with new memory-based test cases.
Summary: This is a small, correct, and well-tested performance improvement. The new overrides faithfully mirror the surrounding synchronous patterns (tab output, delegation, and _tabsPending handling for WriteLine), the ref assembly is kept in sync, and the test coverage exercises both the indentation behavior and the delegation path. The async test refactor to async/await also fixes a latent correctness gap in the tracking writer where LastCalledMethod was previously set synchronously before the awaited operation completed and where the cancelable/ReadOnlyMemory overloads were not tracked. No functional or design concerns found. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 42.1 AIC · ⌖ 10.4 AIC · ⊞ 10K
Saves an array pool rent/return cycle and copying.