Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a null string #17115

ahsonkhan · 2018-03-22T02:52:45Z

Added missing Debug.Asserts to catch when an empty span is being passed to Interop code (which could result in NullReferenceException).
Fixed Span.CompareTo and Span.Equals by adding checks for length == 0 and addressing them ahead of time. Resolved based on the discussion in https://github.com/dotnet/corefx/issues/27350.
Fixed the implementation of Trim, TrimStart, TrimEnd so that its behaviour is identical to the string APIs when trimChars is null/empty.

From https://msdn.microsoft.com/en-us/library/d4tt83f9(v=vs.110).aspx

The string that remains after all occurrences of the characters in the trimChars parameter are removed from the start and end of the current string. If trimChars is null or an empty array, white-space characters are removed instead.

Added tests in corefx: dotnet/corefx#28347

cc @KrzysztofCwalina, @jkotas, @tarekgh, @stephentoub, @joshfree, @VSadov

jkotas · 2018-03-22T03:00:03Z

src/mscorlib/shared/System/Globalization/CompareInfo.Unix.cs

@@ -181,6 +181,8 @@ internal static unsafe int LastIndexOfOrdinalCore(string source, string value, i
        private static unsafe int CompareStringOrdinalIgnoreCase(char* string1, int count1, char* string2, int count2)
        {
            Debug.Assert(!GlobalizationMode.Invariant);
+            Debug.Assert(string1 != null);


The asserts in this file are unnecessary. The Unix globalizations API handle NULL with zero-sized buffer just fine.

(You can keep them if you would like though.)

jkotas · 2018-03-22T03:01:02Z

src/mscorlib/shared/System/Globalization/CompareInfo.cs

@@ -393,13 +393,33 @@ internal int Compare(ReadOnlySpan<char> string1, string string2, CompareOptions

        internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)


virtual is not necessary here. You can remove it.

(It will be more efficient without it.)

jkotas · 2018-03-22T03:01:15Z

src/mscorlib/shared/System/Globalization/CompareInfo.cs

+            }
+            if (string2.IsEmpty)
+                return 1;
+
            return _invariantMode ?
                string.CompareOrdinal(string1, string2) :
                CompareString(string1, string2, CompareOptions.None);
        }

        internal virtual int CompareOptionIgnoreCase(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)


Same here. Virtual is unnecessary.

jkotas · 2018-03-22T03:05:14Z

src/mscorlib/shared/System/Globalization/CompareInfo.cs

@@ -895,12 +917,16 @@ public unsafe virtual int IndexOf(string source, string value, int startIndex, i
        internal virtual int IndexOfOrdinal(ReadOnlySpan<char> source, ReadOnlySpan<char> value, bool ignoreCase)


Virtual is unnecessary.

jkotas · 2018-03-22T03:06:58Z

If trimChars is null or an empty array, white-space characters are removed instead.

I think it is pretty questionable behavior. I would consider it a design bug. I am not sure whether we want to be replicating it...

jkotas · 2018-03-22T03:09:29Z

src/mscorlib/shared/System/Globalization/CompareInfo.cs

@@ -393,13 +393,33 @@ internal int Compare(ReadOnlySpan<char> string1, string string2, CompareOptions

        internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)
        {
+            // Check for empty span or span from a null string
+            if (string1.IsEmpty)


You can just do this as:

if (span.Length == 0 || string2.Length == 0) return string1.Length - string2.Length;

It is how the same check is done in MemoryExtensions.Fast.cs.

jkotas · 2018-03-22T03:10:52Z

src/mscorlib/shared/System/Globalization/CompareInfo.cs

@@ -895,12 +917,16 @@ public unsafe virtual int IndexOf(string source, string value, int startIndex, i
        internal virtual int IndexOfOrdinal(ReadOnlySpan<char> source, ReadOnlySpan<char> value, bool ignoreCase)
        {
            Debug.Assert(!_invariantMode);
+            Debug.Assert(!source.IsEmpty);
+            Debug.Assert(!value.IsEmpty);
            return IndexOfOrdinalCore(source, value, ignoreCase);
        }

        internal unsafe virtual int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options)


virtual is unnecessary.

ahsonkhan · 2018-03-23T03:47:10Z

@dotnet-bot test Windows_NT arm Cross Checked corefx_baseline Build and Test
@dotnet-bot test Windows_NT x64_arm64_altjit Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Windows_NT x86_arm_altjit Checked corefx_baseline
@dotnet-bot test Windows_NT x86 Checked corefx_baseline

ahsonkhan · 2018-03-23T03:47:53Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

Fix CompareTo/Equals when dealing with Span wrapping a null string

b307fbe

ahsonkhan added the area-System.Memory label Mar 22, 2018

ahsonkhan self-assigned this Mar 22, 2018

ahsonkhan mentioned this pull request Mar 22, 2018

Leverage existing string API tests to improve coverage of span-based APIs dotnet/corefx#28347

Merged

jkotas reviewed Mar 22, 2018

View reviewed changes

ahsonkhan added 3 commits March 21, 2018 20:27

Removing unnecessary virtual keyword and address other feedback.

99c7281

Remove unnecessary/incorrect Debug.Asserts.

54d8ef0

Remove more unnecessary/incorrect Debug.Asserts.

d098d40

tarekgh approved these changes Mar 22, 2018

View reviewed changes

ahsonkhan merged commit 48c5b21 into dotnet:master Mar 25, 2018

ahsonkhan deleted the FixNullString branch March 25, 2018 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a null string #17115

Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a null string #17115

ahsonkhan commented Mar 22, 2018 •

edited

Loading

jkotas Mar 22, 2018 •

edited

Loading

jkotas Mar 22, 2018

jkotas Mar 22, 2018

jkotas Mar 22, 2018

jkotas Mar 22, 2018

jkotas Mar 22, 2018

jkotas commented Mar 22, 2018

jkotas Mar 22, 2018

jkotas Mar 22, 2018

ahsonkhan commented Mar 23, 2018

ahsonkhan commented Mar 23, 2018

		@@ -393,13 +393,33 @@ internal int Compare(ReadOnlySpan<char> string1, string string2, CompareOptions

		internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)

		@@ -895,12 +917,16 @@ public unsafe virtual int IndexOf(string source, string value, int startIndex, i
		internal virtual int IndexOfOrdinal(ReadOnlySpan<char> source, ReadOnlySpan<char> value, bool ignoreCase)

Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a null string #17115

Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a null string #17115

Conversation

ahsonkhan commented Mar 22, 2018 • edited Loading

jkotas Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas commented Mar 22, 2018

jkotas Mar 22, 2018

Choose a reason for hiding this comment

jkotas Mar 22, 2018

Choose a reason for hiding this comment

ahsonkhan commented Mar 23, 2018

ahsonkhan commented Mar 23, 2018

ahsonkhan commented Mar 22, 2018 •

edited

Loading

jkotas Mar 22, 2018 •

edited

Loading