Automatic port of System.Buffers triple slash comments #2666
Conversation
|
Area owners, please review: @layomia, @JeremyKuhne, @ahsonkhan |
There was a problem hiding this comment.
I've left a number of suggestions and one comment, @carlossanlop. I haven't quite finished, so I'll review the remainder of this PR shortly.
mairaw
added
🏁 Release: .NET Core 3.0
Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
Co-Authored-By: Maira Wenzel <mairaw@microsoft.com> Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
carlossanlop
added
the
changes-addressed
|
I think that this PR is ready to merge, @carlossanlop, so I'll do that now. |
mairaw
removed
changes-addressed
| <summary>Returns a <see cref="T:System.Span`1" /> to write to that is at least a specified length. | ||
| </summary> | ||
| <returns>A span of at least <paramref name="sizeHint" /> in length. If <paramref name="sizeHint" /> is not provided or is equal to 0, some non-empty buffer is returned.</returns> | ||
| <remarks> |
There was a problem hiding this comment.
There is an issue with how remarks were written in the source which means only one of them got ported.
We should add the rest:
https://github.com/dotnet/corefx/blob/97d5957fc71853961082e22873d926269b291b92/src/Common/src/System/Buffers/ArrayBufferWriter.cs#L142-L150
Same problem with GetMemory.
| @@ -173,7 +173,10 @@ | |||
| <param name="value">The read-only span to be written to <paramref name="writer" />.</param> | |||
| <summary>Writes the contents of <paramref name="value" /> to <paramref name="writer" />.</summary> | |||
| <remarks>To be added.</remarks> | |||
| <exception cref="T:System.ArgumentOutOfRangeException"> | |||
| <paramref name="writer" /> is shorter than <paramref name="value" />. | |||
There was a problem hiding this comment.
nit:
<paramref name="writer" /> doesn't have enough space left to write the <paramref name="value" /> into.| @@ -40,8 +40,8 @@ | |||
| <Docs> | |||
| <param name="reader">To be added.</param> | |||
| <param name="value">To be added.</param> | |||
| <summary>To be added.</summary> | |||
| <returns>To be added.</returns> | |||
| <summary>Reads a big-endian <see cref="T:System.Int16" />.</summary> | |||
There was a problem hiding this comment.
There's something wrong with this file. It doesn't contain all 6 of the relevant APIs, but rather just 3:
https://github.com/dotnet/corefx/blob/97d5957fc71853961082e22873d926269b291b92/src/System.Memory/ref/System.Memory.cs#L396-L404
Is something broken in how the reflection tool is generating this?
Also note, there is Int16, Int64 mismatch happening in the API signature vs the description:
https://docs.microsoft.com/en-us/dotnet/api/system.buffers.sequencereaderextensions?view=netcore-3.0
It is correctly documented in source:
https://github.com/dotnet/corefx/blob/97d5957fc71853961082e22873d926269b291b92/src/System.Memory/src/System/Buffers/SequenceReaderExtensions.Binary.cs#L136-L154
| @@ -445,8 +445,8 @@ | |||
| <ReturnType>System.Int64</ReturnType> | |||
| </ReturnValue> | |||
| <Docs> | |||
| <summary>To be added.</summary> | |||
| <value>To be added.</value> | |||
| <summary>Gets the remaining items in the reader's <see cref="P:System.Buffers.SequenceReader`1.Sequence" />.</summary> | |||
There was a problem hiding this comment.
nit: There is an extra space here.
Gets the remaining -> Gets the remaining
| <summary>To be added.</summary> | ||
| <value>To be added.</value> | ||
| <summary>Gets a <see cref="T:System.Span`1" /> that contains the current segment in the <see cref="P:System.Buffers.SequenceReader`1.Sequence" />.</summary> | ||
| <value>A span that contains the current segment in the sequence.</value> |
There was a problem hiding this comment.
It's a bit odd to read that a span contains anything since it just represents the view.
How it was written in source makes more sense to me:
https://github.com/dotnet/corefx/blob/fec76b0fc06b4fce9e4ac28b9a2bf1101209d984/src/System.Memory/src/System/Buffers/SequenceReader.cs#L59
The current segment in the <see cref="Sequence"/> as a span.
Maybe:
A view of the current segment in the sequence, as a span.
cc @JeremyKuhne
I found a new batch of additional triple slash comments that could be ported automatically.