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.

Sign up for GitHub

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

Jump to bottom

C#: Fix StringExtensions.CompareTo IndexOutOfRangeException #91203

Merged
merged 1 commit into from
May 4, 2024
Merged

C#: Fix StringExtensions.CompareTo IndexOutOfRangeException #91203

merged 1 commit into from
May 4, 2024

Conversation

pirey0
Copy link
Contributor

@pirey0 pirey0 commented Apr 26, 2024 •
edited
Loading

replaced StringExtensions.CompareTo which expects null terminated strings with String.Compare.
fixes #91118

I tested the correction and found that it matches the result from the old method 100% of the time.
Ran test: TestCompareToDifference.cs.txt

@pirey0 pirey0 requested a review from a team as a code owner April 26, 2024 08:16
@akien-mga akien-mga changed the title fixed StringExtensions.CompareTo IndexOutOfRangeException C#: Fix StringExtensions.CompareTo IndexOutOfRangeException Apr 26, 2024
@akien-mga akien-mga added this to the 4.3 milestone Apr 26, 2024
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 26, 2024
@pirey0 pirey0 force-pushed the stringcomp_fix branch 3 times, most recently from facbf67 to 2de3738 Compare April 26, 2024 09:29
raulsntos
raulsntos requested changes Apr 28, 2024
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contributing to the .NET module!

This looks great already, I'm only requesting a few changes based on the discussion in #91118.

modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Show resolved Hide resolved
@pirey0
Copy link
Contributor Author

pirey0 commented Apr 28, 2024 •
edited
Loading

thank you for the improvements! I'll update the documentation and squash the changes

@pirey0 pirey0 requested a review from raulsntos April 29, 2024 14:01
raulsntos
raulsntos reviewed May 1, 2024
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Just a few comments about the documentation to make it sound a bit less repetitive, taking inspiration from the C# documentation.

modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs Outdated Show resolved Hide resolved
@pirey0 @raulsntos
fixed StringExtensions.CompareTo IndexOutOfRangeException
c02540b 
fixes godotengine#91118
marked CompareTo obsolete
dropped CompareTo [-1,1] range

Co-authored-by: Raul Santos <raulsntos@gmail.com>
@pirey0 pirey0 force-pushed the stringcomp_fix branch from 8a09cad to c02540b Compare May 3, 2024 08:14
@pirey0 pirey0 requested a review from raulsntos May 3, 2024 08:15
raulsntos
raulsntos approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Just want to confirm that we agree on the new implementation.

modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs
toIndex++;
}
}
return string.Compare(instance, to, !caseSensitive);
Copy link
Member

Choose a reason for hiding this comment

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

@paulloz Are you OK with using this overload in the new implementation? It will use the current culture: https://github.com/dotnet/runtime/blob/ee93104aa0e7ac77ca7e4c6db9873356a9e3e622/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs#L210

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think it's fine. Anyone is then free to use another locale if they wish anyway 👍

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 not thinking, but calling string.Compare(string, string, bool) (instead of string.Compare(string, string, bool, CultureInfo)) warns with CA1304 and CA1309.

@akien-mga akien-mga merged commit 458106a into godotengine:master May 4, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented May 4, 2024 •
edited
Loading

Thanks! And congrats for your first merged Godot contribution 🎉

paulloz
paulloz reviewed May 4, 2024
View reviewed changes
modules/mono/glue/GodotSharp/GodotSharp/Core/StringExtensions.cs
/// </summary>
/// <param name="instance">The string to compare.</param>
/// <param name="to">The other string to compare.</param>
/// <param name="caseSensitive">
/// If <see langword="true"/>, the comparison will be case sensitive.
/// </param>
/// <returns>-1 if less, 0 if equal and +1 if greater.</returns>
/// <returns>An integer that indicates the lexical relationship between the two comparands.</returns>
[Obsolete("Use string.Compare instead.")]
Copy link
Member

Choose a reason for hiding this comment

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

It didn't occur to me while reviewing earlier, but this means the calls in CasecmpTo and NocasecmpTo now warn with CS0618. It probably should be silenced.

Copy link
Member

Choose a reason for hiding this comment

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

Oh oops, I totally missed this. Yeah, or we can also call the BCL's string.Compare directly.

@paulloz paulloz mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raulsntos raulsntos raulsntos approved these changes

@paulloz paulloz paulloz left review comments

Assignees
No one assigned
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:dotnet
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

StringExtensions.CompareTo leads to IndexOutOfRangeException on non-empty strings
4 participants
@pirey0 @akien-mga @paulloz @raulsntos