-
Notifications
You must be signed in to change notification settings - Fork 750
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
[shared] Remove TagTransformer and improve TagWriter #5602
[shared] Remove TagTransformer and improve TagWriter #5602
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5602 +/- ##
==========================================
+ Coverage 83.38% 85.69% +2.30%
==========================================
Files 297 254 -43
Lines 12531 10979 -1552
==========================================
- Hits 10449 9408 -1041
+ Misses 2082 1571 -511
Flags with carried forward coverage won't be shown. Click here to find out more.
|
case string?[] stringArray: this.WriteStringsToArray(ref arrayState, stringArray, tagValueMaxLength); break; | ||
case bool[] boolArray: this.WriteStructToArray(ref arrayState, boolArray); break; | ||
case byte[] byteArray: this.WriteToArrayCovariant(ref arrayState, byteArray); break; | ||
case short[] shortArray: this.WriteToArrayCovariant(ref arrayState, shortArray); break; |
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.
Why some of the entries like ushortArray
checks were removed. Are these types not expected here?
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.
Looks like it is covered with WriteToArrayTypeChecked
. Is there a benefit of splitting it out?
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.
Kind of an interesting situation. Check out this fiddle: https://dotnetfiddle.net/xLeGFw
Basically runtime treats sbyte[]/byte[], int[]/uint[], short[]/ushort[], and long[]/ulong[] as the same types when it comes to is
. The way the code was written before we wouldn't hit all the branches, whichever was first for one of those pairs would win and we would end up casting wrong. Like sbyte[] falls into byte[] which means values like -128 ((sbyte)0x80) becomes 128 ((byte)0x80).
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.
LGTM
Follow-up to #5476
Relates to #5475
Changes
TagTransformer
as everything is now usingTagWriter
TagWriter
and fix bugs that manifestedReadOnlySpan<char>
forstring
s inTagWriter
to make it possible to bypass allocations when writingchar
s orstring
s which get trimmed due to length limitsMerge requirement checklist