Add missing Stream overloads in DataStreamFromComStream#10857
Add missing Stream overloads in DataStreamFromComStream#10857lonitra merged 2 commits intodotnet:mainfrom
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #10857 +/- ##
===================================================
+ Coverage 72.63168% 73.20244% +0.57075%
===================================================
Files 2934 3080 +146
Lines 620345 633371 +13026
Branches 46946 47403 +457
===================================================
+ Hits 450567 463643 +13076
- Misses 161415 166180 +4765
+ Partials 8363 3548 -4815
Flags with carried forward coverage won't be shown. Click here to find out more. |
JeremyKuhne
left a comment
JeremyKuhne
left a comment
There was a problem hiding this comment.
Thanks for this! Just a minor comment.
|
|
||
| public override int ReadByte() | ||
| { | ||
| Span<byte> oneByteArray = stackalloc byte[1]; |
There was a problem hiding this comment.
You should be able to use the new span constructor, which would be the prefered method:
byte data;
int r = Read(new Span<byte>(ref data));
// ...There was a problem hiding this comment.
Done. I hesitated between the two versions and initially chose this one because it didn't require me to initialise the data field.
There was a problem hiding this comment.
@halgab you could mark this method as [SkipLocalsInit] if you see that it makes it more efficient (look at sharplap.io).
There was a problem hiding this comment.
I tried looking at the il and the assembly in sharplab (here). I can't really interpret the results. I just notice that the asm looks bigger with the attribute. Also, we still have to set data to something despite the attribute.
I guess all those concerns are minor anyway compared to the gains of just defining this method overload properly
dotnet-policy-service
bot
added
the
waiting-author-feedback
dotnet-policy-service
bot
removed
the
waiting-author-feedback
elachlan
added
the
waiting-review
lonitra
added
ready-to-merge
JeremyKuhne
left a comment
JeremyKuhne
left a comment
There was a problem hiding this comment.
This is good, if you want to further optimize see my comment on [SkipLocalsInit]
dotnet-policy-service
bot
removed
the
ready-to-merge
* add missing Stream overloads in DataStreamFromComStream * review feedback
4 participants
Proposed changes
DataStreamFromComStream.ReadByteandWriteByteCustomer Impact
Risk
Test methodology
Microsoft Reviewers: Open in CodeFlow