Repository navigation
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
|
|
||
| T[] array = GC.AllocateUninitializedArray<T>(length); | ||
|
|
||
| for (int i = 0; i < length; i++) |
There was a problem hiding this comment.
| for (int i = 0; i < length; i++) | |
| for (int i = 0; i < array.Length; i++) |
Pretty sure this is needed for the JIT to recognize the loop.
There was a problem hiding this comment.
It's a bit hard to find since it's in a different solution but the existing tests for Array are in src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs.
There was a problem hiding this comment.
So add the new tests to src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs where existing tests for Array are?
| { | ||
| Assert.Throws<ArgumentOutOfRangeException>(() => System.Array.CreateFilled<int>(-1, index => index)); | ||
| Assert.Throws<ArgumentOutOfRangeException>(() => System.Array.CreateFilled<object>(-1, index => new object())); | ||
| } |
There was a problem hiding this comment.
Would be good with tests for the non-factory method too?
There was a problem hiding this comment.
My bad, i didn't find tests when i was implementing it :)
|
Are there any places in this repo that can be improved by switching to use these APIs? |
Wherever this "pattern" is used: T[] array = new T[n];
Array.Fill(array, value);For example:
|
|
Can you please update these places to use this API in this PR? |
Done, I replaced all occurrences matching that "pattern". Future idea (out of scope for this PR): analyzer + code fix for this "pattern". |
| @@ -495,8 +495,7 @@ public static void DefaultFilledIndexOfAny_TwoString() | |||
|
|
|||
| for (int length = 0; length < byte.MaxValue; length++) | |||
| { | |||
| var a = new string[length]; | |||
| Array.Fill(a, ""); | |||
| var a = Array.CreateFilled(length, ""); | |||
There was a problem hiding this comment.
| var a = Array.CreateFilled(length, ""); | |
| string[] a = Array.CreateFilled(length, ""); |
The repo coding conventions allow var only when the type is explicitly named on the right-side.
https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md
(fix all instances)
There was a problem hiding this comment.
The repo coding conventions allow
varonly when the type is explicitly named on the right-side.https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md
(fix all instances)
My bad :)
It should be fixed.
| /// <returns>A new array of the specified length, with each element initialized by <paramref name="factory"/>.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="factory"/> is <see langword="null"/>.</exception> | ||
| /// <exception cref="ArgumentOutOfRangeException"><paramref name="length"/> is negative.</exception> | ||
| public static T[] CreateFilled<T>(int length, Func<int, T> factory) |
There was a problem hiding this comment.
I am not sure whether it is wise to implement Func<int, T> overload at this point since there are no real-world examples so far where it would be beneficial to use it.
There was a problem hiding this comment.
I am fine with it being deferred, particularly if we believe functional interfaces might come in the next release or two. I know Andy has a language proposal up for it now.
There was a problem hiding this comment.
I did have usecases for it in the past personally, but I'd have more of them with it using functional interfaces.
This branch has not been deployed
5 participants
Fixes #121477