Add Array.Fill apis #8137
Add Array.Fill apis #8137
Conversation
| for (int i = 0; i < array.Length; i++) | ||
| { | ||
| array[i] = value; | ||
| } |
There was a problem hiding this comment.
NOTE: This is just the initial implementation which gets things up and running. This can probably be optimized later to be specialized for different types, e.g. use initblk for bytes, or write 16 bytes at a time for other primitives. Or, implementing this natively, so for classes we only incur the array write type-check once. But that is work for a later PR.
There was a problem hiding this comment.
FYI this is going to mean that my Fill extension method will no longer be selected
@jnm2 This is a static method, so there should be no conflict.
| } | ||
| } | ||
|
|
||
| public static void Fill<T>(T[] array, int startIndex, int count, T value) |
There was a problem hiding this comment.
@terrajobst, this API shape is a little concerning to me. Suppose T == int. Then going from the shorter overload to the longer overload changes the interpretation of the int as the second argument from value to startIndex. Did we consider reordering the args to:
public static void Fill<T>(T[] array, T value);
public static void Fill<T>(T[] array, T value, int startIndex, int count);? I do understand that it's common to have array/offset/count all next to each other as args; maybe that consistency is more important?
There was a problem hiding this comment.
@stephentoub, good point. I just checked and the generic IndexOf accepts (T[], T, int, int). It sounds reasonable to put the value first.
| ThrowHelper.ThrowCountArgumentOutOfRange_ArgumentOutOfRange_Count(); | ||
| } | ||
|
|
||
| for (int i = startIndex, end = startIndex + count; i < end; i++) |
There was a problem hiding this comment.
Does this give different code than for (int i = startIndex; i < startIndex + count; i++)
There was a problem hiding this comment.
@danmosemsft Probably not. That was more of a style thing, but I can change it to the form you've suggested.
|
@jamesqo next step is on you right? just making sure we're not waiting for each other. |
|
@danmosemsft Yep, sorry, have been busy lately. I should be able to make the change later today. |
|
No problem @jamesqo just keeping the PR's moving. |
|
@danmosemsft This is OK for merge now. |
|
@jamesqo were you going to address @stephentoub 's comment about param ordering? |
|
@danmosemsft Strange, I thought I had already taken care of that. Done as well. |
|
Test Windows_NT x86 ryujit Checked Build and Test |
Add Array.Fill apis Commit migrated from dotnet/coreclr@3653534
This PR adds the initial implementation of
Array.Fill, which has been approved in dotnet/corefx#6695.FYI @weshaggard @jkotas @benaadams @justinvp