Return empty array if length is zero#16529
Conversation
| if (_length == 0) | ||
| { | ||
| #if FEATURE_PORTABLE_SPAN | ||
| arraySegment = new ArraySegment<T>(Array<T>.Empty); |
There was a problem hiding this comment.
I do not think Array<T>.Empty is portable. It should be SpanHelpers.PerTypeValues<T>.EmptyArray
| arraySegment = ArraySegment<T>.Empty; | ||
| #endif // FEATURE_PORTABLE_SPAN | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Is doing this check here what we want? Seems like it should instead be done at the end, just before returning false, so that we still call through an OwnedMemory if there is one, and preserve array object identity if this wraps an array, in case that's needed in some situation.
As part of resolving this, we did also add tests in corefx. Also, do we want to make a similar change to MemoryMarshal.TryGetArray? |
|
cc: @ahsonkhan , @KrzysztofCwalina , @davidfowl |
| arraySegment = new ArraySegment<T>(arr, _index, _length); | ||
| return true; | ||
| } | ||
| else if (_length == 0) |
There was a problem hiding this comment.
With "else", this won't kick in for an owned memory that doesn't wrap an array. Is that what's desired?
|
@stephentoub, should I also make this change in CoreFX or will it be automatically moved? I can close this PR and add it in CoreFX with tests in one PR, instead of having two. |
It'll be automatically mirrored.
No, keep this one. You won't be able to merge tests in corefx until it's using a coreclr build with the change. |
ghost
mentioned this pull request
|
I have added the case in MemoryMarshal and tests in CoreFX dotnet/corefx#27430 |
ahsonkhan
left a comment
ahsonkhan
left a comment
There was a problem hiding this comment.
LGTM. Thanks for fixing this.
Can you please build coreclr locally, build corefx using your local coreclr build and run the corefx tests you just added here (dotnet/corefx#27430) to confirm they pass?
See https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits for more details. If you have questions on how to do that, please feel free to ask.
* Return empty array if length is zero * Return true * Add support for portable system * Use portable span helper instead of Array.Empty * Move to end * Remove else * Return empty array if length is 0 in MemoryMarshal Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Return empty array if length is zero * Return true * Add support for portable system * Use portable span helper instead of Array.Empty * Move to end * Remove else * Return empty array if length is 0 in MemoryMarshal Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Return empty array if length is zero * Return true * Add support for portable system * Use portable span helper instead of Array.Empty * Move to end * Remove else * Return empty array if length is 0 in MemoryMarshal Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Return empty array if length is zero * Return true * Add support for portable system * Use portable span helper instead of Array.Empty * Move to end * Remove else * Return empty array if length is 0 in MemoryMarshal Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Return empty array if length is zero * Return true * Add support for portable system * Use portable span helper instead of Array.Empty * Move to end * Remove else * Return empty array if length is 0 in MemoryMarshal Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
4 participants
Resolves https://github.com/dotnet/corefx/issues/27412