Skip to content
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

Fix Array.Sort in the presence of nulls #41234

Merged
merged 2 commits into from
Aug 23, 2020
Merged

Conversation

svick
Copy link
Contributor

@svick svick commented Aug 23, 2020

Fixes #41235.

This was introduced by https://github.com/dotnet/runtime/pull/35297/files#diff-54c667fe59347842036834f5b31c5411L513-R477. Notice that the first while loop stays == null, but the second one incorrectly changes from != null to == null.

I think this is a fairly serious bug that should be fixed in 5.0, but as I understand it, master already targets 6.0. Should I rebase and retarget this PR to the release/5.0 branch?

cc: @stephentoub

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@svick svick changed the title [WIP] Fix Array.Sort in the presence of nulls Fix Array.Sort in the presence of nulls Aug 23, 2020
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svick Thank you!

@jkotas
Copy link
Member

jkotas commented Aug 23, 2020

Should I rebase and retarget this PR to the release/5.0 branch?

All changes go to master. We backport from master as necessary.

@jkotas
Copy link
Member

jkotas commented Aug 23, 2020

Test failures are #38159 and #41078

@jkotas
Copy link
Member

jkotas commented Aug 23, 2020

/backport to release/5.0

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/221163831

@danmoseley
Copy link
Member

Shoukd we have more test combinations, given this involves the complexity of pivots, etc?

@jeffhandley
Copy link
Member

I added a tracking issue #41376 for adding more test combinations.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
DDolG added a commit to DDolG/runtime that referenced this pull request Apr 26, 2022
*Added tests for sorting arrays

*Separated tests for integer from string

*Added comments for integers and strings
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array.Sort ( .NET 5.0 Preview 7 ) generates wrong result for a sample data set.
5 participants