Skip to content

Conversation

prozolic
Copy link

This PR optimizes the bounds checking logic for index validation in ImmutableArray<T>.ElementAtOrDefault.
The current implementation uses index < 0 || index >= immutableArray.Length, but I replaced this with unsigned comparison (uint)index >= (uint)immutableArray.Length to reduce bounds checks.
This pattern is already used in Enumerable.ElementAtOrDefault, making this change safe and consistent.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 14, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2025
@stephentoub
Copy link
Member

@EgorBo, the JIT is already successfully eliding the bounds check here based on the comparisons, but it's still ending up with two branches:
SharpLab
Is that expected?

@EgorBo
Copy link
Member

EgorBo commented Aug 14, 2025

Is that expected?

Yes, these have different semantics: if index is negative - one of them returns default, the other fails with NRE

@stephentoub
Copy link
Member

Ah, right, because ImmutableArray.Length just returns array.Length, and array may be null in the case of default(ImmutableArray).

@stephentoub
Copy link
Member

stephentoub commented Aug 14, 2025

@prozolic, thanks, but as this would change behavior when consuming default(ImmutableArray), we can't take the PR. Appreciate the attempt, though.

@prozolic
Copy link
Author

You're right, default(ImmutableArray) was an oversight on my part. Thank you for pointing that out.

@prozolic prozolic deleted the ElementAtOrDefault branch August 14, 2025 22:39
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants