Implement BinaryReader.ReadExactly#106238
Conversation
Implements the BinaryReader.ReadExactly method, as proposed in dotnet#101614 Fix dotnet#101614
|
Note regarding the |
|
Note regarding the |
|
@dotnet-policy-bot agree |
adamsitnik
left a comment
There was a problem hiding this comment.
The PR is almost ready, PTAL at my comments.
@Log234 thank you for your contribution!
| { | ||
| using (var stream = CreateStream()) | ||
| { | ||
| var source = new byte[sourceSize].AsSpan(); |
There was a problem hiding this comment.
nit: AsSpan is needed only for test projects that target older monikers like Full .NET Framework (because Span<T> is not part of mscorlib.dll in Full Framework, so we could not have added an implicit cast operator from array to span for Full Framework).
| var source = new byte[sourceSize].AsSpan(); | |
| byte[] source = new byte[sourceSize]; |
There was a problem hiding this comment.
I see, thanks for the explanation. I have updated the tests to remove the .AsSpan() calls.
| return result; | ||
| } | ||
|
|
||
| public virtual void ReadExactly(Span<byte> buffer) |
There was a problem hiding this comment.
The BinaryReader source files appear almost not have any tripple slash comments documenting the methods. I would of course be happy to add documentation, but I am unsure if the documentation of these methods is supposed to go somewhere else or if I am supposed to break with the existing "convention" of not documenting these methods.
Please add the tripple slash comments here. We have an internal porting process where we sporadically run a command line app that feeds the new comments to api docs. Sample outcome: dotnet/dotnet-api-docs#10210
You can use the wording from Stream.ReadExactly as an inspiration.
There was a problem hiding this comment.
Thanks for the clarification, I have updated the method with a triple slash comment, based on the Stream.ReadExactly method as you suggested.
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you @Log234 !
BTW dotnet/runtime is currently in a temporary mode where we merge to main only PRs that are a must have for the .NET 9 release. As soon as our main branch becomes .NET 10, I am going to merge your PR and it will be released in .NET 10 Preview 1.
|
It seems that there's a similar method: runtime/src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs Lines 456 to 474 in dcf9814 |
This PR implements the
BinaryReader.ReadExactly(Span<byte> buffer)method, as proposed in #101614, as well as some new tests to cover the new method.The BinaryReader source files appear almost not have any tripple slash comments documenting the methods. I would of course be happy to add documentation, but I am unsure if the documentation of these methods is supposed to go somewhere else or if I am supposed to break with the existing "convention" of not documenting these methods.
Fixes #101614