Skip to content

Eagerly determine when a set is unsortable #3998

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

Closed
stevenaw opened this issue Nov 23, 2021 · 0 comments · Fixed by #4048
Closed

Eagerly determine when a set is unsortable #3998

stevenaw opened this issue Nov 23, 2021 · 0 comments · Fixed by #4048
Assignees
Milestone

Comments

@stevenaw
Copy link
Member

While trying to optimize collectiontally by pre-sorting items in #3841, an edge case was hit where a Tuple<T1, T2> may implement IComparable even if it's underlying properties didn't. This could cause an exception at runtime. A workaround was done in the PR (#3881) to catch the exception and fallback to a slower path.

#3976 was subsequently filed around an edge-case of this edge-case, where a tuple which has its first property implement IComparable and it's second doesn't could throw an exception at such a point that it left the array in a corrupt state. The workaround in PR #3978 was to copy the array prior to sorting.

We should consider proactively detecting these cases and falling back to the slow path. This will have a few benefits:

  1. Simpler code
  2. Likely lower memory overhead due to not needing to copy the array internally
  3. Better surfacing of issues in calling code by allowing exceptions in user-defined IComparers to halt the comparison entirely
@stevenaw stevenaw self-assigned this Nov 27, 2021
@stevenaw stevenaw changed the title Proactively determine when a set is unsortable Eagerly determine when a set is unsortable Nov 27, 2021
@rprouse rprouse added this to the 3.13.3 milestone Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants