Fix CompareTo/Equals when dealing with Empty Span or Span wrapping a …

…null string (#17115) * Fix CompareTo/Equals when dealing with Span wrapping a null string * Removing unnecessary virtual keyword and address other feedback. * Remove unnecessary/incorrect Debug.Asserts. * Remove more unnecessary/incorrect Debug.Asserts.
dotnet · Mar 25, 2018 · 48c5b21 · 48c5b21
1 parent 19f470b
commit 48c5b21
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 4 deletions.
diff --git a/src/mscorlib/shared/System/Globalization/CompareInfo.Unix.cs b/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);
+            Debug.Assert(string2 != null);
 
             return Interop.Globalization.CompareStringOrdinalIgnoreCase(string1, count1, string2, count2);
         }

diff --git a/src/mscorlib/shared/System/Globalization/CompareInfo.Windows.cs b/src/mscorlib/shared/System/Globalization/CompareInfo.Windows.cs
@@ -40,6 +40,8 @@ private static unsafe int FindStringOrdinal(
             bool bIgnoreCase)
         {
             Debug.Assert(!GlobalizationMode.Invariant);
+            Debug.Assert(stringSource != null);
+            Debug.Assert(value != null);
 
             fixed (char* pSource = stringSource)
             fixed (char* pValue = value)
@@ -62,6 +64,8 @@ private static unsafe int FindStringOrdinal(
             bool bIgnoreCase)
         {
             Debug.Assert(!GlobalizationMode.Invariant);
+            Debug.Assert(!source.IsEmpty);
+            Debug.Assert(!value.IsEmpty);
 
             fixed (char* pSource = &MemoryMarshal.GetReference(source))
             fixed (char* pValue = &MemoryMarshal.GetReference(value))
@@ -165,6 +169,8 @@ private unsafe int GetHashCodeOfStringCore(string source, CompareOptions options
         private static unsafe int CompareStringOrdinalIgnoreCase(char* string1, int count1, char* string2, int count2)
         {
             Debug.Assert(!GlobalizationMode.Invariant);
+            Debug.Assert(string1 != null);
+            Debug.Assert(string2 != null);
 
             // Use the OS to compare and then convert the result to expected value by subtracting 2 
             return Interop.Kernel32.CompareStringOrdinal(string1, count1, string2, count2, true) - 2;
@@ -185,6 +191,7 @@ private unsafe int CompareString(ReadOnlySpan<char> string1, string string2, Com
             fixed (char* pString1 = &MemoryMarshal.GetReference(string1))
             fixed (char* pString2 = &string2.GetRawStringData())
             {
+                Debug.Assert(pString1 != null);
                 int result = Interop.Kernel32.CompareStringEx(
                                     pLocaleName,
                                     (uint)GetNativeCompareFlags(options),
@@ -217,6 +224,8 @@ private unsafe int CompareString(ReadOnlySpan<char> string1, ReadOnlySpan<char>
             fixed (char* pString1 = &MemoryMarshal.GetReference(string1))
             fixed (char* pString2 = &MemoryMarshal.GetReference(string2))
             {
+                Debug.Assert(pString1 != null);
+                Debug.Assert(pString2 != null);
                 int result = Interop.Kernel32.CompareStringEx(
                                     pLocaleName,
                                     (uint)GetNativeCompareFlags(options),
@@ -245,6 +254,8 @@ private unsafe int FindString(
                     int* pcchFound)
         {
             Debug.Assert(!_invariantMode);
+            Debug.Assert(!lpStringSource.IsEmpty);
+            Debug.Assert(!lpStringValue.IsEmpty);
 
             string localeName = _sortHandle != IntPtr.Zero ? null : _sortName;
 
@@ -277,6 +288,8 @@ private unsafe int FindString(
             int* pcchFound)
         {
             Debug.Assert(!_invariantMode);
+            Debug.Assert(lpStringSource != null);
+            Debug.Assert(lpStringValue != null);
 
             string localeName = _sortHandle != IntPtr.Zero ? null : _sortName;
 
@@ -572,6 +585,7 @@ private unsafe SortKey CreateSortKey(String source, CompareOptions options)
         private static unsafe bool IsSortable(char* text, int length)
         {
             Debug.Assert(!GlobalizationMode.Invariant);
+            Debug.Assert(text != null);
 
             return Interop.Kernel32.IsNLSDefinedString(Interop.Kernel32.COMPARE_STRING, 0, IntPtr.Zero, text, length);
         }

diff --git a/src/mscorlib/shared/System/Globalization/CompareInfo.cs b/src/mscorlib/shared/System/Globalization/CompareInfo.cs
@@ -391,15 +391,23 @@ internal int Compare(ReadOnlySpan<char> string1, string string2, CompareOptions
             return CompareString(string1, string2, options);
         }
 
-        internal virtual int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)
+        internal int CompareOptionNone(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)
         {
+            // Check for empty span or span from a null string
+            if (string1.Length == 0 || string2.Length == 0)
+                return string1.Length - string2.Length;
+
             return _invariantMode ?
                 string.CompareOrdinal(string1, string2) :
                 CompareString(string1, string2, CompareOptions.None);
         }
 
-        internal virtual int CompareOptionIgnoreCase(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)
+        internal int CompareOptionIgnoreCase(ReadOnlySpan<char> string1, ReadOnlySpan<char> string2)
         {
+            // Check for empty span or span from a null string
+            if (string1.Length == 0 || string2.Length == 0)
+                return string1.Length - string2.Length;
+
             return _invariantMode ?
                 CompareOrdinalIgnoreCase(string1, string2) :
                 CompareString(string1, string2, CompareOptions.IgnoreCase);
@@ -892,15 +900,19 @@ public unsafe virtual int IndexOf(string source, string value, int startIndex, i
             return IndexOfCore(source, value, startIndex, count, options, null);
         }
 
-        internal virtual int IndexOfOrdinal(ReadOnlySpan<char> source, ReadOnlySpan<char> value, bool ignoreCase)
+        internal 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)
+        internal unsafe int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options)
         {
             Debug.Assert(!_invariantMode);
+            Debug.Assert(!source.IsEmpty);
+            Debug.Assert(!value.IsEmpty);
             return IndexOfCore(source, value, options, null);
         }
 

diff --git a/src/mscorlib/shared/System/MemoryExtensions.cs b/src/mscorlib/shared/System/MemoryExtensions.cs
@@ -113,6 +113,7 @@ public static ReadOnlySpan<char> TrimEnd(this ReadOnlySpan<char> span, char trim
         /// </summary>
         /// <param name="span">The source span from which the characters are removed.</param>
         /// <param name="trimChars">The span which contains the set of characters to remove.</param>
+        /// <remarks>If <paramref name="trimChars"/> is empty, white-space characters are removed instead.</remarks>
         public static ReadOnlySpan<char> Trim(this ReadOnlySpan<char> span, ReadOnlySpan<char> trimChars)
         {
             return span.TrimStart(trimChars).TrimEnd(trimChars);
@@ -124,8 +125,14 @@ public static ReadOnlySpan<char> Trim(this ReadOnlySpan<char> span, ReadOnlySpan
         /// </summary>
         /// <param name="span">The source span from which the characters are removed.</param>
         /// <param name="trimChars">The span which contains the set of characters to remove.</param>
+        /// <remarks>If <paramref name="trimChars"/> is empty, white-space characters are removed instead.</remarks>
         public static ReadOnlySpan<char> TrimStart(this ReadOnlySpan<char> span, ReadOnlySpan<char> trimChars)
         {
+            if (trimChars.IsEmpty)
+            {
+                return span.TrimStart();
+            }
+
             int start = 0;
             for (; start < span.Length; start++)
             {
@@ -147,8 +154,14 @@ public static ReadOnlySpan<char> TrimStart(this ReadOnlySpan<char> span, ReadOnl
         /// </summary>
         /// <param name="span">The source span from which the characters are removed.</param>
         /// <param name="trimChars">The span which contains the set of characters to remove.</param>
+        /// <remarks>If <paramref name="trimChars"/> is empty, white-space characters are removed instead.</remarks>
         public static ReadOnlySpan<char> TrimEnd(this ReadOnlySpan<char> span, ReadOnlySpan<char> trimChars)
         {
+            if (trimChars.IsEmpty)
+            {
+                return span.TrimEnd();
+            }
+
             int end = span.Length - 1;
             for (; end >= 0; end--)
             {