Skip to content

Conversation

@tarekgh
Copy link
Member

@tarekgh tarekgh commented Sep 2, 2024

This issue addressing #106828.

In Globalization invariant mode when comparing string with case insensitive option, should satisfy the rule: if s1 < s2 and s2 < s3, then s1 < s3. The change here is fixing the string comparison to ensure this rule.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh added this to the 10.0.0 milestone Sep 2, 2024
@tarekgh
Copy link
Member Author

tarekgh commented Sep 2, 2024

@ericstj I am willing to port this fix to 9.0 release, the comment #106828 (comment) is helpful why I want to do that. The change is small and safe with low risk.

}

return (int)codePointA - (int)codePointB;
return (int)aUpper - (int)bUpper;
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised we didn't have cases where this returned the wrong result entirely, for instance when comparing a single upper case to lower case. Do we need more test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

This case scoped to the globalization invariant mode. The test I added here should be good enough to catch the cases we need to fix.

Note, we have the right implementation in the other places, like

and

Copy link
Member

Choose a reason for hiding this comment

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

I was just imagining that it might give the wrong answers (let alone ordering) for things like:
a B -> 97 - 65 = 32 <-- incorrect
A b -> 65 - 98 = -33
A B -> 65 - 66 = -1

And I was surprised we didn't have a theory test somewhere for such basic comparisons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I understand what you are saying but does it really matter much testing single character string or a string with multiple characters as the case I added? if you think this is worth adding I can do it.

@tarekgh
Copy link
Member Author

tarekgh commented Sep 3, 2024

/backport to release/9.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2024

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10689249576

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Sep 6, 2024
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
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.

2 participants