Implement IEnumerator<T> on ref struct enumerators #106309

AlexRadch · 2024-08-13T02:29:40Z

ghost · 2024-08-13T02:29:46Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2024-08-13T02:29:47Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.Memory/ref/System.Memory.cs

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs

src/libraries/System.Private.CoreLib/src/System/Span.cs

src/libraries/System.Private.CoreLib/src/System/Text/SpanLineEnumerator.cs

huoyaoyuan · 2024-08-22T05:15:08Z

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorSpanHelpers.cs

@@ -222,7 +222,7 @@ public static void ValidateStrides(ReadOnlySpan<nint> strides, ReadOnlySpan<nint
        /// <param name="length">The length of the TensorSpan we are iterating over.</param>
        public static void AdjustIndexes(int curIndex, nint addend, Span<nint> curIndexes, scoped ReadOnlySpan<nint> length)
        {
-            if (addend <= 0 || curIndex < 0)
+            if (addend <= 0 || curIndex < 0 || length[curIndex] <= 0)


What's hitting this? Will this affect behavior of other public API?

It removes Access violation when length[curIndex] is zero. Now code throws an Access violation for an empty tensor.
That was incorrect behavior. I add tests for empty tensor.

@michaelgsharp

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs

.../System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.EnumerateMatches.cs

Calling MoveNext after it's already returned false is a programmer error. It is not something we need to optimize for.

…umerators

…untime into RefStructEnumerators

Removed redundant assertions for match properties after calling Assert.False(enumerator.MoveNext()).

Updated test methods in ReadOnlyTensorSpanTests.cs to replace instances of TensorSpan with ReadOnlyTensorSpan. This ensures tests validate the behavior of ReadOnlyTensorSpan, including exception handling and constructor functionality. Adjustments made for handling empty and multi-dimensional spans appropriately.

- Updated `Slice` method in `ReadOnlyTensorSpan<T>` to use `ThrowHelper.ThrowIndexOutOfRangeException()` for consistent error handling. - Removed unnecessary `using System.Reflection;` directive from `TensorSpanTests.cs`. - Adjusted `TensorSpan_SliceTest` to call `TensorSpan_TestEnumerator(spanInt);` after defining `spanInt` and updated exception checks. - Introduced a `value` variable in `TensorSpan_TestEnumerator<T>` to accurately restore the enumerator's current value for improved assertions.

stephentoub · 2025-02-06T04:35:21Z

src/libraries/System.Memory/tests/ReadOnlySpan/GetEnumerator.cs

+            static void TestGI<TEnumerator>(TEnumerator enumerator) where TEnumerator : IEnumerator<int>, allows ref struct
+            {
+                Assert.False(enumerator.MoveNext());
+                try { _ = enumerator.Current; } catch (Exception) { }


What are all of these try/catches trying to validate? Is it trying to test that an exception gets thrown? If so, there should be asserts validating that.

stephentoub · 2025-02-06T04:35:56Z

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs

+            /// <summary>Gets the element at the current position of the enumerator.</summary>
+            T IEnumerator<T>.Current
+            {
+                [MethodImpl(MethodImplOptions.AggressiveInlining)]


These shouldn't need AggressiveInlining on them

stephentoub · 2025-02-06T04:37:07Z

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs

@@ -484,7 +486,8 @@ internal Enumerator(ReadOnlyTensorSpan<T> span)
                _span = span;
                _items = -1;
                _curIndexes = new nint[_span.Rank];
-                _curIndexes[_span.Rank - 1] = -1;
+                if (_span.Rank > 0)
+                    _curIndexes[_span.Rank - 1] = -1;


Suggested change

_curIndexes[_span.Rank - 1] = -1;

{

_curIndexes[_span.Rank - 1] = -1;

}

stephentoub · 2025-02-06T04:37:19Z

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs

+                _items = -1;
+                _curIndexes.Clear();
+                if (_span.Rank > 0)
+                    _curIndexes[_span.Rank - 1] = -1;


Suggested change

_curIndexes[_span.Rank - 1] = -1;

{

_curIndexes[_span.Rank - 1] = -1;

}

stephentoub · 2025-02-06T04:38:26Z

src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs

+            /// <summary>Gets the element at the current position of the enumerator.</summary>
+            T IEnumerator<T>.Current
+            {
+                [MethodImpl(MethodImplOptions.AggressiveInlining)]


These also should not need AggressiveInlining.

AlexRadch added 4 commits August 11, 2024 02:30

SpanSplitEnumerator, ReadOnlySpanEnumerator, SpanEnumerator

ac589fb

SpanLineEnumerator, SpanRuneEnumerator

501f876

ReadOnlyTensorSpan, TensorSpan

9772213

ValueMatchEnumerator, ValueSplitEnumerator

5f8e874

ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Aug 13, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 13, 2024

huoyaoyuan reviewed Aug 13, 2024

View reviewed changes

@huoyaoyuan Review

ffc350e

am11 added area-System.Memory and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 13, 2024

This was referenced Aug 13, 2024

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

msbuild crashes with "MSB0001: Internal MSBuild Error: must be valid" dotnet/dnceng#3304

Open

ref\System.Text.RegularExpressions

21154bc

This was referenced Aug 13, 2024

MSBuild crashing in the build #92290

Open

System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest failure - time out, unexpected exit code #106206

Closed

Merge branch 'main' into RefStructEnumerators

99794bb

This was referenced Aug 14, 2024

nativeaot/SmokeTests/Exceptions failing with Assertion failed: (n_heaps <= heap_number) || !gc_t_join.joined() #103839

Closed

Microsoft.Extensions.Hosting.Internal.HostTests.StartCanBeCancelled test failure #106135

Open

huoyaoyuan reviewed Aug 22, 2024

View reviewed changes

stephentoub reviewed Sep 5, 2024

View reviewed changes

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs Show resolved Hide resolved

stephentoub reviewed Sep 5, 2024

View reviewed changes

.../System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.EnumerateMatches.cs Outdated Show resolved Hide resolved

Revert some changes Regex.EnumerateMatches.cs after code review

2f60a8b

Calling MoveNext after it's already returned false is a programmer error. It is not something we need to optimize for.

build-analysis bot mentioned this pull request Sep 11, 2024

System.Numerics.Tensors.Tests.TensorSpanTests.TensorExtensionsTwoSpanInFloatOut fails with IndexOutOfRangeException #107254

Open

build-analysis bot mentioned this pull request Sep 6, 2024

Microsoft.Extensions.SourceGeneration.Configuration.Binder.Tests.ConfigurationBindingGeneratorTests fails on CI #107381

Closed

AlexRadch added 2 commits January 8, 2025 20:34

Merge remote-tracking branch 'remotes/upstream/main' into RefStructEn…

6e487a6

…umerators

Merge branch 'RefStructEnumerators' of https://github.com/AlexRadch/r…

c5cf26d

…untime into RefStructEnumerators

Refactor match assertions in EnumerateMatches tests

043af5b

Removed redundant assertions for match properties after calling Assert.False(enumerator.MoveNext()).

build-analysis bot mentioned this pull request Jan 9, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

This was referenced Jan 9, 2025

ModuleNotFoundError: No module named 'pkg_resources' dotnet/dnceng#4756

Closed

Build on Windows Fails sometimes with fatal error C1090: PDB API call failed #48070

Open

[browser][MT] MutexTests - operation does not support unaligned accesses #96191

Open

AlexRadch requested review from huoyaoyuan and stephentoub January 10, 2025 03:54

Merge branch 'main' into RefStructEnumerators

ccc2e4b

stephentoub reviewed Feb 6, 2025

View reviewed changes

Merge branch 'main' into RefStructEnumerators

c78b1ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IEnumerator<T> on ref struct enumerators #106309

Implement IEnumerator<T> on ref struct enumerators #106309

AlexRadch commented Aug 13, 2024

ghost commented Aug 13, 2024

ghost commented Aug 13, 2024

huoyaoyuan Aug 22, 2024

AlexRadch Aug 22, 2024

stephentoub Sep 5, 2024

stephentoub Feb 6, 2025

stephentoub Feb 6, 2025

stephentoub Feb 6, 2025

stephentoub Feb 6, 2025

stephentoub Feb 6, 2025

Implement IEnumerator<T> on ref struct enumerators #106309

Are you sure you want to change the base?

Implement IEnumerator<T> on ref struct enumerators #106309

Conversation

AlexRadch commented Aug 13, 2024

ghost commented Aug 13, 2024

ghost commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment