-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Modernize XmlUTF8TextWriter with u8/inline out's. #75812
Conversation
7f4186f
to
05e3dfb
Compare
If the goal is to copy to the beginning of the buffer, you'd just do |
if (value.Length < bufferLength) | ||
{ | ||
byte[] buffer = GetBuffer(value.Length, out int offset); | ||
value.CopyTo(new Span<byte>(buffer, offset, buffer.Length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
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))
.
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? |
@StephenMolloy and @mconnew can you please take a look? |
There was a problem hiding this 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.
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 abyte[]
thanvalue.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.