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

Modernize XmlUTF8TextWriter with u8/inline out's. #75812

Merged
merged 2 commits into from
Mar 27, 2023

Conversation

jlennox
Copy link
Contributor

@jlennox jlennox commented Sep 18, 2022

I couldn't find any tests for this area of code. I am not certain there are any. I have performed no testing. This was done because while browsing I noticed this code would benefit from some modernizing.

Is there a better method to copy a ReadOnlySpan<byte> to a byte[] than value.CopyTo(new Span<byte>(buffer, offset, buffer.Length));?

I followed the if (value.Length < bufferLength) semantics but as currently used, that condition will always be true.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 18, 2022
@jlennox jlennox force-pushed the jlennox/modernize-utf8-xml-writer branch from 7f4186f to 05e3dfb Compare September 18, 2022 19:56
@stephentoub
Copy link
Member

Is there a better method to copy a ReadOnlySpan to a byte[] than value.CopyTo(new Span(buffer, offset, buffer.Length));?

If the goal is to copy to the beginning of the buffer, you'd just do value.CopyTo(buffer) and rely on the implicit conversion from an array to a span. If the goal is to copy to some non-0 offset, then you can do buffer.AsSpan(offset). You'd almost never see new Span<byte>(buffer, offset, buffer.Length), as that would fail for any offset other than 0 (since the length is the full length of the buffer).

if (value.Length < bufferLength)
{
byte[] buffer = GetBuffer(value.Length, out int offset);
value.CopyTo(new Span<byte>(buffer, offset, buffer.Length));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
value.CopyTo(new Span<byte>(buffer, offset, buffer.Length));
value.CopyTo(buffer.AsSpan(offset));

If the code in this PR is passing tests, it seems like we're missing some tests, as a non-0 offset value here will result in exceptions.

@@ -267,8 +267,7 @@ protected void WriteUTF8Chars(byte[] chars, int charOffset, int charCount)
{
if (charCount < bufferLength)
{
int offset;
byte[] buffer = GetBuffer(charCount, out offset);
byte[] buffer = GetBuffer(charCount, out int offset);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this WriteUTF8Chars method now that there's the WriteUTF8Bytes method below? Any remaining call sites could be changed from WriteUTF8Chars(value, offset, count) to WriteUTF8Chars(value.AsSpan(offset, count)).

@stephentoub
Copy link
Member

Thanks for the PR. It seems like this might be duplicating changes already being made in https://github.com/dotnet/runtime/pull/71478/files#diff-b500e28b981633d459d67a89d678a1f71e05f9497d1ac49859ff86b53b3ab836R225?

@HongGit
Copy link
Contributor

HongGit commented Feb 2, 2023

@StephenMolloy and @mconnew can you please take a look?

Copy link
Member

@StephenMolloy StephenMolloy left a comment

Choose a reason for hiding this comment

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

Addressed a couple points. Looks good.

@StephenMolloy
Copy link
Member

StephenMolloy commented Mar 27, 2023

Test failures are unrelated. (See #83901 and #83986 which are currently preventing clean CI runs.)

@StephenMolloy StephenMolloy merged commit 47cd24e into dotnet:main Mar 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants