Cleanup/optimize some String.Compare overloads, part 2 (dotnet/coreclr#6891)
authorJames Ko <jamesqko@gmail.com>
Mon, 29 Aug 2016 14:32:43 +0000 (10:32 -0400)
committerJan Kotas <jkotas@microsoft.com>
Mon, 29 Aug 2016 14:32:43 +0000 (07:32 -0700)
* Change signature of CompareOrdinalEx and move arg validation to the call sites
* Change thrown param name from count -> length
* Rename nativeCompareOrdinalEx -> CompareOrdinalHelper

Commit migrated from https://github.com/dotnet/coreclr/commit/255cb7355da9cbfda84bdd09e388ef660eddffe3

src/coreclr/src/classlibnative/bcltype/stringnative.cpp
src/coreclr/src/classlibnative/bcltype/stringnative.h
src/coreclr/src/mscorlib/src/System/Globalization/CompareInfo.cs
src/coreclr/src/mscorlib/src/System/String.cs
src/coreclr/src/vm/ecalllist.h

index 46be714..1e6b132 100644 (file)
@@ -285,7 +285,7 @@ FCIMPLEND
         STRINGREF value; INT32 thisOffset;} _compareOrdinalArgsEx;
 ==============================================================================*/
 
-FCIMPL5(INT32, COMString::CompareOrdinalEx, StringObject* strA, INT32 indexA, StringObject* strB, INT32 indexB, INT32 count)
+FCIMPL6(INT32, COMString::CompareOrdinalEx, StringObject* strA, INT32 indexA, INT32 countA, StringObject* strB, INT32 indexB, INT32 countB)
 {
     FCALL_CONTRACT;
 
@@ -294,41 +294,16 @@ FCIMPL5(INT32, COMString::CompareOrdinalEx, StringObject* strA, INT32 indexA, St
     DWORD *strAChars, *strBChars;
     int strALength, strBLength;
 
-    // This runtime test is handled in the managed wrapper.
+    // These runtime tests are handled in the managed wrapper.
     _ASSERTE(strA != NULL && strB != NULL);
-
-    //If any of our indices are negative throw an exception.
-    if (count<0)
-    {
-        FCThrowArgumentOutOfRange(W("count"), W("ArgumentOutOfRange_NegativeCount"));
-    }
-    if (indexA < 0)
-    {
-        FCThrowArgumentOutOfRange(W("indexA"), W("ArgumentOutOfRange_Index"));
-    }
-    if (indexB < 0)
-    {
-        FCThrowArgumentOutOfRange(W("indexB"), W("ArgumentOutOfRange_Index"));
-    }
+    _ASSERTE(indexA >= 0 && indexB >= 0);
+    _ASSERTE(countA >= 0 && countB >= 0);
 
     strA->RefInterpretGetStringValuesDangerousForGC((WCHAR **) &strAChars, &strALength);
     strB->RefInterpretGetStringValuesDangerousForGC((WCHAR **) &strBChars, &strBLength);
 
-    int countA = count;
-    int countB = count;
-
-    //Do a lot of range checking to make sure that everything is kosher and legit.
-    if (count  > (strALength - indexA)) {
-        countA = strALength - indexA;
-        if (countA < 0)
-            FCThrowArgumentOutOfRange(W("indexA"), W("ArgumentOutOfRange_Index"));
-    }
-
-    if (count > (strBLength - indexB)) {
-        countB = strBLength - indexB;
-        if (countB < 0)
-            FCThrowArgumentOutOfRange(W("indexB"), W("ArgumentOutOfRange_Index"));
-    }
+    _ASSERTE(countA <= strALength - indexA);
+    _ASSERTE(countB <= strBLength - indexB);
 
     // Set up the loop variables.
     strAChars = (DWORD *) ((WCHAR *) strAChars + indexA);
index 6d0d413..a4d962d 100644 (file)
@@ -59,7 +59,7 @@ public:
 
     static FCDECL2(INT32, FCCompareOrdinalIgnoreCaseWC, StringObject* strA, __in_z INT8 *strB);
 
-    static FCDECL5(INT32, CompareOrdinalEx, StringObject* strA, INT32 indexA, StringObject* strB, INT32 indexB, INT32 count);
+    static FCDECL6(INT32, CompareOrdinalEx, StringObject* strA, INT32 indexA, INT32 countA, StringObject* strB, INT32 indexB, INT32 countB);
 
     static FCDECL4(INT32, LastIndexOfCharArray, StringObject* thisRef, CHARArray* valueRef, INT32 startIndex, INT32 count );
 
index 0498a01..0b14f05 100644 (file)
@@ -528,8 +528,7 @@ namespace System.Globalization {
 
             if (options == CompareOptions.Ordinal)
             {
-                return CompareOrdinal(string1, offset1, length1,
-                                      string2, offset2, length2);
+                return string.CompareOrdinalHelper(string1, offset1, length1, string2, offset2, length2);
             }
             return InternalCompareString(this.m_dataHandle, this.m_handleOrigin, this.m_sortName, 
                                          string1, offset1, length1, 
@@ -537,18 +536,6 @@ namespace System.Globalization {
                                          GetNativeCompareFlags(options));
         }
 
-        [System.Security.SecurityCritical]
-        private static int CompareOrdinal(string string1, int offset1, int length1, string string2, int offset2, int length2)
-        {
-            int result = String.nativeCompareOrdinalEx(string1, offset1, string2, offset2,
-                                                       (length1 < length2 ? length1 : length2));
-            if ((length1 != length2) && result == 0)
-            {
-                return (length1 > length2 ? 1 : -1);
-            }
-            return (result);
-        }
-
         ////////////////////////////////////////////////////////////////////////
         //
         //  IsPrefix
index 2956c42..86a367a 100644 (file)
@@ -307,7 +307,7 @@ namespace System {
         // native call to COMString::CompareOrdinalEx
         [System.Security.SecurityCritical]  // auto-generated
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal static extern int nativeCompareOrdinalEx(String strA, int indexA, String strB, int indexB, int count);
+        internal static extern int CompareOrdinalHelper(String strA, int indexA, int countA, String strB, int indexB, int countB);
 
         //This will not work in case-insensitive mode for any character greater than 0x80.  
         //We'll throw an ArgumentException.
@@ -2102,84 +2102,61 @@ namespace System {
         // beginning at indexB of the same length.
         //
         [Pure]
-        public static int Compare(String strA, int indexA, String strB, int indexB, int length) {
-            int lengthA = length;
-            int lengthB = length;
-
-            if (strA!=null) {
-                if (strA.Length - indexA < lengthA) {
-                  lengthA = (strA.Length - indexA);
-                }
-            }
-
-            if (strB!=null) {
-                if (strB.Length - indexB < lengthB) {
-                    lengthB = (strB.Length - indexB);
-                }
-            }
-            return CultureInfo.CurrentCulture.CompareInfo.Compare(strA, indexA, lengthA, strB, indexB, lengthB, CompareOptions.None);
+        public static int Compare(String strA, int indexA, String strB, int indexB, int length)
+        {
+            // NOTE: It's important we call the boolean overload, and not the StringComparison
+            // one. The two have some subtly different behavior (see notes in the former).
+            return Compare(strA, indexA, strB, indexB, length, ignoreCase: false);
         }
 
-
         // Determines whether two string regions match.  The substring of strA beginning
         // at indexA of length count is compared with the substring of strB
         // beginning at indexB of the same length.  Case sensitivity is determined by the ignoreCase boolean.
         //
         [Pure]
-        public static int Compare(String strA, int indexA, String strB, int indexB, int length, bool ignoreCase) {
+        public static int Compare(String strA, int indexA, String strB, int indexB, int length, bool ignoreCase)
+        {
+            // Ideally we would just forward to the string.Compare overload that takes
+            // a StringComparison parameter, and just pass in CurrentCulture/CurrentCultureIgnoreCase.
+            // That function will return early if an optimization can be applied, e.g. if
+            // (object)strA == strB && indexA == indexB then it will return 0 straightaway.
+            // There are a couple of subtle behavior differences that prevent us from doing so
+            // however:
+            // - string.Compare(null, -1, null, -1, -1, StringComparison.CurrentCulture) works
+            //   since that method also returns early for nulls before validation. It shouldn't
+            //   for this overload.
+            // - Since we originally forwarded to CompareInfo.Compare for all of the argument
+            //   validation logic, the ArgumentOutOfRangeExceptions thrown will contain different
+            //   parameter names.
+            // Therefore, we have to duplicate some of the logic here.
+
             int lengthA = length;
             int lengthB = length;
-
-            if (strA!=null) {
-                if (strA.Length - indexA < lengthA) {
-                  lengthA = (strA.Length - indexA);
-                }
+            
+            if (strA != null)
+            {
+                lengthA = Math.Min(lengthA, strA.Length - indexA);
             }
 
-            if (strB!=null) {
-                if (strB.Length - indexB < lengthB) {
-                    lengthB = (strB.Length - indexB);
-                }
+            if (strB != null)
+            {
+                lengthB = Math.Min(lengthB, strB.Length - indexB);
             }
 
-            if (ignoreCase) {
-                return CultureInfo.CurrentCulture.CompareInfo.Compare(strA, indexA, lengthA, strB, indexB, lengthB, CompareOptions.IgnoreCase);
-            }
-            return CultureInfo.CurrentCulture.CompareInfo.Compare(strA, indexA, lengthA, strB, indexB, lengthB, CompareOptions.None);
+            var options = ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None;
+            return CultureInfo.CurrentCulture.CompareInfo.Compare(strA, indexA, lengthA, strB, indexB, lengthB, options);
         }
-    
+
         // Determines whether two string regions match.  The substring of strA beginning
         // at indexA of length length is compared with the substring of strB
         // beginning at indexB of the same length.  Case sensitivity is determined by the ignoreCase boolean,
         // and the culture is set by culture.
         //
         [Pure]
-        public static int Compare(String strA, int indexA, String strB, int indexB, int length, bool ignoreCase, CultureInfo culture) {
-            if (culture == null) {
-                throw new ArgumentNullException("culture");
-            }
-            Contract.EndContractBlock();
-
-            int lengthA = length;
-            int lengthB = length;
-
-            if (strA!=null) {
-                if (strA.Length - indexA < lengthA) {
-                  lengthA = (strA.Length - indexA);
-                }
-            }
-
-            if (strB!=null) {
-                if (strB.Length - indexB < lengthB) {
-                    lengthB = (strB.Length - indexB);
-                }
-            }
-    
-            if (ignoreCase) {
-                return culture.CompareInfo.Compare(strA,indexA,lengthA, strB, indexB, lengthB,CompareOptions.IgnoreCase);
-            } else {
-                return culture.CompareInfo.Compare(strA,indexA,lengthA, strB, indexB, lengthB,CompareOptions.None);
-            }
+        public static int Compare(String strA, int indexA, String strB, int indexB, int length, bool ignoreCase, CultureInfo culture)
+        {
+            var options = ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None;
+            return Compare(strA, indexA, strB, indexB, length, culture, options);
         }
 
 
@@ -2270,7 +2247,7 @@ namespace System {
                     return CultureInfo.InvariantCulture.CompareInfo.Compare(strA, indexA, lengthA, strB, indexB, lengthB, CompareOptions.IgnoreCase);
 
                 case StringComparison.Ordinal:
-                    return nativeCompareOrdinalEx(strA, indexA, strB, indexB, length);
+                    return CompareOrdinalHelper(strA, indexA, lengthA, strB, indexB, lengthB);
 
                 case StringComparison.OrdinalIgnoreCase:
 #if FEATURE_COREFX_GLOBALIZATION
@@ -2350,16 +2327,48 @@ namespace System {
         //
         [Pure]
         [System.Security.SecuritySafeCritical]  // auto-generated
-        public static int CompareOrdinal(String strA, int indexA, String strB, int indexB, int length) {
-           if (strA == null || strB == null) {
-                if ((Object)strA==(Object)strB) { //they're both null;
+        public static int CompareOrdinal(String strA, int indexA, String strB, int indexB, int length)
+        {
+            if (strA == null || strB == null)
+            {
+                if (object.ReferenceEquals(strA, strB))
+                {
+                    // They're both null
                     return 0;
                 }
 
-                return (strA==null)? -1 : 1; //-1 if A is null, 1 if B is null.
+                return strA == null ? -1 : 1;
+            }
+
+            // COMPAT: Checking for nulls should become before the arguments are validated,
+            // but other optimizations which allow us to return early should come after.
+
+            if (length < 0)
+            {
+                throw new ArgumentOutOfRangeException("length", Environment.GetResourceString("ArgumentOutOfRange_NegativeCount"));
+            }
+
+            if (indexA < 0 || indexB < 0)
+            {
+                string paramName = indexA < 0 ? "indexA" : "indexB";
+                throw new ArgumentOutOfRangeException(paramName, Environment.GetResourceString("ArgumentOutOfRange_Index"));
+            }
+            
+            int lengthA = Math.Min(length, strA.Length - indexA);
+            int lengthB = Math.Min(length, strB.Length - indexB);
+
+            if (lengthA < 0 || lengthB < 0)
+            {
+                string paramName = lengthA < 0 ? "indexA" : "indexB";
+                throw new ArgumentOutOfRangeException(paramName, Environment.GetResourceString("ArgumentOutOfRange_Index"));
+            }
+
+            if (length == 0 || (object.ReferenceEquals(strA, strB) && indexA == indexB))
+            {
+                return 0;
             }
 
-            return nativeCompareOrdinalEx(strA, indexA, strB, indexB, length);
+            return CompareOrdinalHelper(strA, indexA, lengthA, strB, indexB, lengthB);
         }
 
 
@@ -2413,7 +2422,7 @@ namespace System {
                     return CultureInfo.InvariantCulture.CompareInfo.IsSuffix(this, value, CompareOptions.IgnoreCase);                    
 
                 case StringComparison.Ordinal:
-                    return this.Length < value.Length ? false : (nativeCompareOrdinalEx(this, this.Length -value.Length, value, 0, value.Length) == 0);
+                    return this.Length < value.Length ? false : (CompareOrdinalHelper(this, this.Length - value.Length, value.Length, value, 0, value.Length) == 0);
 
                 case StringComparison.OrdinalIgnoreCase:
 #if FEATURE_COREFX_GLOBALIZATION
index c693acd..19e0f34 100644 (file)
@@ -224,7 +224,7 @@ FCFuncStart(gStringFuncs)
     FCIntrinsic("get_Length", COMString::Length, CORINFO_INTRINSIC_StringLength)
     FCIntrinsic("get_Chars", COMString::GetCharAt, CORINFO_INTRINSIC_StringGetChar)
     FCFuncElement("IsAscii", COMString::IsAscii)
-    FCFuncElement("nativeCompareOrdinalEx", COMString::CompareOrdinalEx)
+    FCFuncElement("CompareOrdinalHelper", COMString::CompareOrdinalEx)
     FCFuncElement("IndexOfAny", COMString::IndexOfCharArray)
     FCFuncElement("LastIndexOfAny", COMString::LastIndexOfCharArray)
     FCFuncElementSig("ReplaceInternal", &gsig_IM_Str_Str_RetStr, COMString::ReplaceString)