-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Optimize Stack<T>'s enumerator #117328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize Stack<T>'s enumerator #117328
Conversation
There's a non-trivial amount of unnecessary overhead/complexity in `Stack<T>`'s enumerator. This streamlines it. (It does also change the undefined behavior of using Current erroneously. We can put back most of its current semantics if we really care. This change puts it on a plan closer to `List<T>`.)
|
Tagging subscribers to this area: @dotnet/area-system-collections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Streamlines Stack<T>'s enumerator by removing extra state checks and aligning its behavior more closely with List<T>, resulting in a significant performance improvement.
- Renames test properties related to undefined
Currentbehavior. - Refactors
Enumeratorto use a single index and simplerMoveNext/Currentlogic.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Collections/tests/Generic/Stack/Stack.Tests.cs | Renamed test property for undefined Current behavior |
| src/libraries/System.Collections/tests/Generic/Stack/Stack.Generic.Tests.cs | Renamed test property for undefined Current behavior |
| src/libraries/System.Collections/src/System/Collections/Generic/Stack.cs | Simplified Enumerator implementation, updated index handling and removed old guards |
Comments suppressed due to low confidence (1)
src/libraries/System.Collections/tests/Generic/Stack/Stack.Tests.cs:27
- The property name looks like it has a typo. It should be
Enumerator_Empty_Current_UndefinedOperation_Throws(with a trailing 's') to match the naming pattern and the corresponding base property.
protected override bool Enumerator_Empty_Current_UndefinedOperation_Throw => true;
There's a non-trivial amount of unnecessary overhead/complexity in
Stack<T>'s enumerator. This streamlines it.(It does also change the undefined behavior of using Current erroneously. We can put back most of its current semantics if we really care. This change puts it on a plan closer to
List<T>.)