From 54f167900792a139d1d582998c5fbe5b7186bcb3 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 3 Apr 2020 16:19:20 -0700 Subject: [PATCH 01/30] Create spanified overloads of CompareInfo APIs --- .../Globalization/CompareInfo.Invariant.cs | 49 ++ .../Globalization/CompareInfo.Windows.cs | 101 +++- .../src/System/Globalization/CompareInfo.cs | 458 ++++++++++++++++++ .../System/MemoryExtensions.Globalization.cs | 18 + .../src/System/ThrowHelper.cs | 6 + 5 files changed, 631 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs index 922a471d3fb3cc..61f0f6886f1540 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs @@ -269,5 +269,54 @@ private static void InvariantCreateSortKeyOrdinalIgnoreCase(ReadOnlySpan s sortKey = sortKey.Slice(sizeof(ushort)); } } + + private int GetSortKeyInvariant(ReadOnlySpan source, Span sortKey, CompareOptions options) + { + Debug.Assert(GlobalizationMode.Invariant); + Debug.Assert((options & ValidCompareMaskOffFlags) == 0); + + // Make sure the destination buffer is large enough to hold the source projection. + // Using unsigned arithmetic below also checks for buffer overflow since the incoming + // length is always a non-negative signed integer. + + if ((uint)sortKey.Length < (uint)source.Length * sizeof(char)) + { + throw new ArgumentException( + paramName: nameof(sortKey), + message: SR.Arg_BufferTooSmall); + } + + if ((options & CompareOptions.IgnoreCase) == 0) + { + InvariantCreateSortKeyOrdinal(source, sortKey); + } + else + { + InvariantCreateSortKeyOrdinalIgnoreCase(source, sortKey); + } + + return source.Length * sizeof(char); + } + + private int GetSortKeyLengthInvariant(ReadOnlySpan source, CompareOptions options) + { + Debug.Assert(GlobalizationMode.Invariant); + Debug.Assert((options & ValidCompareMaskOffFlags) == 0); + + // In invariant mode, sort keys are simply a byte projection of the source input, + // optionally with casing modifications. We need to make sure we don't overflow + // while computing the length. + + int byteLength = source.Length * sizeof(char); + + if (byteLength < 0) + { + throw new ArgumentException( + paramName: nameof(source), + message: SR.ArgumentOutOfRange_GetByteCountOverflow); + } + + return byteLength; + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index cbc7ac00209d27..16a29590c2cd60 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -400,7 +400,6 @@ private unsafe bool StartsWith(ReadOnlySpan source, ReadOnlySpan pre { Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(!source.IsEmpty); Debug.Assert(!prefix.IsEmpty); Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); @@ -525,6 +524,106 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options) return new SortKey(this, source, options, keyData); } + private unsafe int GetSortKeyCore(ReadOnlySpan source, Span sortKey, CompareOptions options) + { + Debug.Assert(!GlobalizationMode.Invariant); + Debug.Assert((options & ValidCompareMaskOffFlags) == 0); + + // LCMapStringEx doesn't allow cchDest = 0 unless we're trying to query + // the total number of bytes necessary. + + if (sortKey.IsEmpty) + { + throw new ArgumentException( + paramName: nameof(sortKey), + message: SR.Argument_CannotBeEmptySpan); + } + + uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options); + + // LCMapStringEx doesn't support passing cchSrc = 0, so if given an empty span + // we'll instead normalize to a null-terminated empty string and pass -1 as + // the length to indicate that the implicit null terminator should be used. + + int sourceLength = source.Length; + if (sourceLength == 0) + { + source = string.Empty; + sourceLength = -1; + } + + int actualSortKeyLength; + + fixed (char* pSource = &MemoryMarshal.GetReference(source)) + fixed (byte* pSortKey = &MemoryMarshal.GetReference(sortKey)) + { + Debug.Assert(pSource != null); + Debug.Assert(pSortKey != null); + actualSortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName, + flags, + pSource, sourceLength, + pSortKey, sortKey.Length, + null, null, _sortHandle); + } + + if (actualSortKeyLength <= 0) + { + Debug.Assert(actualSortKeyLength == 0, "LCMapStringEx should never return a negative value."); + + // This could fail for a variety of reasons, including NLS being unable + // to allocate a temporary buffer large enough to hold intermediate state, + // or the destination buffer being too small. + + throw new ArgumentException(SR.Arg_ExternalException); + } + + Debug.Assert(actualSortKeyLength <= sortKey.Length); + return actualSortKeyLength; + } + + private unsafe int GetSortKeyLengthCore(ReadOnlySpan source, CompareOptions options) + { + Debug.Assert(!GlobalizationMode.Invariant); + Debug.Assert((options & ValidCompareMaskOffFlags) == 0); + + uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options); + + // LCMapStringEx doesn't support passing cchSrc = 0, so if given an empty span + // we'll instead normalize to a null-terminated empty string and pass -1 as + // the length to indicate that the implicit null terminator should be used. + + int sourceLength = source.Length; + if (sourceLength == 0) + { + source = string.Empty; + sourceLength = -1; + } + + int sortKeyLength; + + fixed (char* pSource = &MemoryMarshal.GetReference(source)) + { + Debug.Assert(pSource != null); + sortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName, + flags, + pSource, sourceLength, + null, 0, + null, null, _sortHandle); + } + + if (sortKeyLength <= 0) + { + Debug.Assert(sortKeyLength == 0, "LCMapStringEx should never return a negative value."); + + // This could fail for a variety of reasons, including NLS being unable + // to allocate a temporary buffer large enough to hold intermediate state. + + throw new ArgumentException(SR.Arg_ExternalException); + } + + return sortKeyLength; + } + private static unsafe bool IsSortable(char* text, int length) { Debug.Assert(!GlobalizationMode.Invariant); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 57c6f81afeb2c8..9db72dc0bd9b01 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -157,6 +157,32 @@ public static unsafe bool IsSortable(string text) } } + /// + /// Indicates whether a specified Unicode string is sortable. + /// + /// A string of zero or more Unicode characters. + /// + /// if is non-empty and contains + /// only sortable Unicode characters; otherwise, . + /// + public static unsafe bool IsSortable(ReadOnlySpan text) + { + if (text.Length == 0) + { + return false; + } + + if (GlobalizationMode.Invariant) + { + return true; + } + + fixed (char* pChar = text) + { + return IsSortable(pChar, text.Length); + } + } + [OnDeserializing] private void OnDeserializing(StreamingContext ctx) { @@ -451,6 +477,69 @@ public int Compare(string? string1, int offset1, int length1, string? string2, i return CompareString(span1, span2, options); } + /// + /// Compares two strings. + /// + /// The first string to compare. + /// The second string to compare. + /// The to use during the comparison. + /// + /// Zero if and are equal; + /// or a negative value if sorts before ; + /// or a positive value if sorts after . + /// + /// + /// contains an unsupported combination of flags. + /// + public int Compare(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options = CompareOptions.None) + { + if ((options & ValidCompareMaskOffFlags) == 0) + { + // Common case: caller is attempting to perform linguistic comparison. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) + { + return CompareString(string1, string2, options); + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; + } + } + else + { + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + throw new ArgumentException( + paramName: nameof(options), + message: SR.Argument_InvalidFlag); + } + } + + ReturnOrdinal: + return string1.SequenceCompareTo(string2); + + ReturnOrdinalIgnoreCase: + return CompareOrdinalIgnoreCase(string1, string2); + } + /// /// CompareOrdinalIgnoreCase compare two string ordinally with ignoring the case. /// it assumes the strings are Ascii string till we hit non Ascii character in strA or strB and then we continue the comparison by @@ -728,6 +817,77 @@ internal bool IsPrefix(ReadOnlySpan source, ReadOnlySpan prefix, Com return StartsWith(source, prefix, options); } + /// + /// Determines whether a string starts with a specific prefix. + /// + /// The string to search within. + /// The prefix to attempt to match at the start of . + /// The to use during the match. + /// + /// if occurs at the start of ; + /// otherwise, . + /// + /// + /// contains an unsupported combination of flags. + /// + public bool IsPrefixNew(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options = CompareOptions.None) + { + // The empty string is trivially a prefix of every other string. For compat with + // earlier versions of the Framework we'll early-exit here before validating the + // 'options' argument. + + if (prefix.IsEmpty) + { + return true; + } + + if ((options & ValidIndexMaskOffFlags) == 0) + { + // Common case: caller is attempting to perform a linguistic search. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) + { + return StartsWith(source, prefix, options); + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; + } + } + else + { + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + throw new ArgumentException( + paramName: nameof(options), + message: SR.Argument_InvalidFlag); + } + } + + ReturnOrdinal: + return source.StartsWith(prefix); + + ReturnOrdinalIgnoreCase: + return source.StartsWithOrdinalIgnoreCase(prefix); + } + public bool IsPrefix(string source, string prefix) { return IsPrefix(source, prefix, 0); @@ -791,6 +951,77 @@ internal bool IsSuffix(ReadOnlySpan source, ReadOnlySpan suffix, Com return EndsWith(source, suffix, options); } + /// + /// Determines whether a string ends with a specific suffix. + /// + /// The string to search within. + /// The suffix to attempt to match at the end of . + /// The to use during the match. + /// + /// if occurs at the end of ; + /// otherwise, . + /// + /// + /// contains an unsupported combination of flags. + /// + public bool IsSuffixNew(ReadOnlySpan source, ReadOnlySpan suffix, CompareOptions options = CompareOptions.None) + { + // The empty string is trivially a suffix of every other string. For compat with + // earlier versions of the Framework we'll early-exit here before validating the + // 'options' argument. + + if (suffix.IsEmpty) + { + return true; + } + + if ((options & ValidIndexMaskOffFlags) == 0) + { + // Common case: caller is attempting to perform a linguistic search. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) + { + return EndsWith(source, suffix, options); + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; + } + } + else + { + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + throw new ArgumentException( + paramName: nameof(options), + message: SR.Argument_InvalidFlag); + } + } + + ReturnOrdinal: + return source.EndsWith(suffix); + + ReturnOrdinalIgnoreCase: + return source.EndsWithOrdinalIgnoreCase(suffix); + } + public bool IsSuffix(string source, string suffix) { return IsSuffix(source, suffix, 0); @@ -966,6 +1197,75 @@ public unsafe int IndexOf(string source, string value, int startIndex, int count return IndexOf(source, value, startIndex, count, options, null); } + /// + /// Searches for the first occurrence of a substring within a source string. + /// + /// The string to search within. + /// The substring to locate within . + /// The to use during the search. + /// + /// The zero-based index into where the substring + /// first appears; or -1 if cannot be found within . + /// + /// + /// contains an unsupported combination of flags. + /// + public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) + { + if ((options & ValidIndexMaskOffFlags) == 0) + { + // Common case: caller is attempting to perform a linguistic search. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) + { + if (value.IsEmpty) + { + return 0; // Empty target string trivially occurs at index 0 of every search space. + } + else + { + return IndexOfCore(source, value, options, null /* matchLengthPtr */, fromBeginning: true); + } + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; + } + } + else + { + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + throw new ArgumentException( + paramName: nameof(options), + message: SR.Argument_InvalidFlag); + } + } + + ReturnOrdinal: + return source.IndexOf(value); + + ReturnOrdinalIgnoreCase: + return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: true); + } + internal int IndexOfOrdinalIgnoreCase(ReadOnlySpan source, ReadOnlySpan value) { Debug.Assert(!GlobalizationMode.Invariant); @@ -975,6 +1275,33 @@ internal int IndexOfOrdinalIgnoreCase(ReadOnlySpan source, ReadOnlySpan source, ReadOnlySpan value, bool fromBeginning) + { + if (value.IsEmpty) + { + // Empty target string trivially appears at all indexes of all search spaces. + + return (fromBeginning) ? 0 : source.Length; + } + + if (value.Length > source.Length) + { + // A non-linguistic search compares chars directly against one another, so large + // target strings can never be found inside small search spaces. + + return -1; + } + + if (GlobalizationMode.Invariant) + { + return InvariantIndexOf(source, value, ignoreCase: true, fromBeginning); + } + else + { + return IndexOfOrdinalCore(source, value, ignoreCase: true, fromBeginning); + } + } + internal int LastIndexOfOrdinal(ReadOnlySpan source, ReadOnlySpan value, bool ignoreCase) { Debug.Assert(!GlobalizationMode.Invariant); @@ -1360,6 +1687,75 @@ public int LastIndexOf(string source, string value, int startIndex, int count, C return LastIndexOfCore(source, value, startIndex, count, options); } + /// + /// Searches for the last occurrence of a substring within a source string. + /// + /// The string to search within. + /// The substring to locate within . + /// The to use during the search. + /// + /// The zero-based index into where the substring + /// last appears; or -1 if cannot be found within . + /// + /// + /// contains an unsupported combination of flags. + /// + public unsafe int LastIndexOfNew(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) + { + if ((options & ValidIndexMaskOffFlags) == 0) + { + // Common case: caller is attempting to perform a linguistic search. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) + { + if (value.IsEmpty) + { + return source.Length; // Empty target string trivially occurs at the last index of every search space. + } + else + { + return IndexOfCore(source, value, options, null /* matchLengthPtr */, fromBeginning: false); + } + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; + } + } + else + { + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + throw new ArgumentException( + paramName: nameof(options), + message: SR.Argument_InvalidFlag); + } + } + + ReturnOrdinal: + return source.LastIndexOf(value); + + ReturnOrdinalIgnoreCase: + return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: false); + } + private static int LastIndexOfOrdinal(string source, string value, int startIndex, int count, bool ignoreCase) { Debug.Assert(!string.IsNullOrEmpty(source)); @@ -1411,6 +1807,68 @@ public SortKey GetSortKey(string source) return CreateSortKey(source, CompareOptions.None); } + /// + /// Computes a sort key over the specified input. + /// + /// The text over which to compute the sort key. + /// The buffer into which to write the resulting sort key. + /// The used for computing the sort key. + /// The number of bytes written to . + /// + /// Use to query the required size of . + /// It is acceptable to provide a larger-than-necessary output buffer to this method. + /// + /// + /// is too small to contain the resulting sort key; + /// or contains an unsupported flag; + /// or cannot be processed using the desired + /// under the current . + /// + public int GetSortKey(ReadOnlySpan source, Span sortKey, CompareOptions options = CompareOptions.None) + { + if ((options & ValidCompareMaskOffFlags) != 0) + { + ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidFlag, ExceptionArgument.options); + } + + if (GlobalizationMode.Invariant) + { + return GetSortKeyInvariant(source, sortKey, options); + } + else + { + return GetSortKeyCore(source, sortKey, options); + } + } + + /// + /// Returns the length (in bytes) of the sort key that would be produced from the specified input. + /// + /// The text over which to compute the sort key. + /// The used for computing the sort key. + /// The length (in bytes) of the sort key. + /// + /// contains an unsupported flag; + /// or cannot be processed using the desired + /// under the current . + /// + public int GetSortKeyLength(ReadOnlySpan source, CompareOptions options = CompareOptions.None) + { + if ((options & ValidCompareMaskOffFlags) != 0) + { + ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidFlag, ExceptionArgument.options); + } + + if (GlobalizationMode.Invariant) + { + return GetSortKeyLengthInvariant(source, options); + } + else + { + return GetSortKeyLengthCore(source, options); + } + } + public override bool Equals(object? value) { return value is CompareInfo otherCompareInfo diff --git a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs index fb339f76cc71bc..a2f243b4b31c24 100644 --- a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs +++ b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; +using Internal.Runtime.CompilerServices; namespace System { @@ -358,6 +359,16 @@ public static bool EndsWith(this ReadOnlySpan span, ReadOnlySpan val CultureInfo.CurrentCulture.CompareInfo.IsSuffix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static bool EndsWithOrdinalIgnoreCase(this ReadOnlySpan span, ReadOnlySpan value) + { + return value.Length <= span.Length + && CompareInfo.EqualsOrdinalIgnoreCase( + ref Unsafe.Add(ref MemoryMarshal.GetReference(span), span.Length - value.Length), + ref MemoryMarshal.GetReference(value), + value.Length); + } + /// /// Determines whether the beginning of the matches the specified when compared using the specified option. /// @@ -391,6 +402,13 @@ public static bool StartsWith(this ReadOnlySpan span, ReadOnlySpan v CultureInfo.CurrentCulture.CompareInfo.IsPrefix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static bool StartsWithOrdinalIgnoreCase(this ReadOnlySpan span, ReadOnlySpan value) + { + return value.Length <= span.Length + && CompareInfo.EqualsOrdinalIgnoreCase(ref MemoryMarshal.GetReference(span), ref MemoryMarshal.GetReference(value), value.Length); + } + /// /// Returns an enumeration of from the provided span. /// diff --git a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs index 8ddc596a909fe6..2d776129fbed76 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs @@ -688,6 +688,8 @@ private static string GetArgumentName(ExceptionArgument argument) return "codePoint"; case ExceptionArgument.str: return "str"; + case ExceptionArgument.options: + return "options"; default: Debug.Fail("The enum value is not defined, please check the ExceptionArgument Enum."); return ""; @@ -840,6 +842,8 @@ private static string GetResourceString(ExceptionResource resource) return SR.Arg_TypeNotSupported; case ExceptionResource.Argument_SpansMustHaveSameLength: return SR.Argument_SpansMustHaveSameLength; + case ExceptionResource.Argument_InvalidFlag: + return SR.Argument_InvalidFlag; default: Debug.Fail("The enum value is not defined, please check the ExceptionResource Enum."); return ""; @@ -939,6 +943,7 @@ internal enum ExceptionArgument year, codePoint, str, + options, } // @@ -1011,5 +1016,6 @@ internal enum ExceptionResource Rank_MultiDimNotSupported, Arg_TypeNotSupported, Argument_SpansMustHaveSameLength, + Argument_InvalidFlag, } } From 864c28a7e871f05fef77737e792b380a9694ca64 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 10:30:34 -0700 Subject: [PATCH 02/30] Plumb through more CompareInfo APIs --- .../src/System/Globalization/CompareInfo.cs | 56 +++++++++++++++++++ .../src/System/Text/Rune.cs | 33 ++++++++--- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 9db72dc0bd9b01..93f8a11d0f8c05 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.Serialization; +using System.Text; using System.Text.Unicode; using Internal.Runtime.CompilerServices; @@ -183,6 +184,21 @@ public static unsafe bool IsSortable(ReadOnlySpan text) } } + /// + /// Indicates whether a specified is sortable. + /// + /// A Unicode scalar value. + /// + /// if is a sortable Unicode scalar + /// value; otherwise, . + /// + public static bool IsSortable(Rune value) + { + Span valueAsUtf16 = stackalloc char[Rune.MaxUtf16CharsPerRune]; + int charCount = value.EncodeToUtf16(valueAsUtf16); + return IsSortable(valueAsUtf16.Slice(0, charCount)); + } + [OnDeserializing] private void OnDeserializing(StreamingContext ctx) { @@ -1266,6 +1282,26 @@ public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: true); } + /// + /// Searches for the first occurrence of a within a source string. + /// + /// The string to search within. + /// The to locate within . + /// The to use during the search. + /// + /// The zero-based index into where + /// first appears; or -1 if cannot be found within . + /// + /// + /// contains an unsupported combination of flags. + /// + public int IndexOfNew(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) + { + Span valueAsUtf16 = stackalloc char[Rune.MaxUtf16CharsPerRune]; + int charCount = value.EncodeToUtf16(valueAsUtf16); + return IndexOfNew(source, valueAsUtf16.Slice(0, charCount), options); + } + internal int IndexOfOrdinalIgnoreCase(ReadOnlySpan source, ReadOnlySpan value) { Debug.Assert(!GlobalizationMode.Invariant); @@ -1756,6 +1792,26 @@ public unsafe int LastIndexOfNew(ReadOnlySpan source, ReadOnlySpan v return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: false); } + /// + /// Searches for the last occurrence of a within a source string. + /// + /// The string to search within. + /// The to locate within . + /// The to use during the search. + /// + /// The zero-based index into where + /// last appears; or -1 if cannot be found within . + /// + /// + /// contains an unsupported combination of flags. + /// + public unsafe int LastIndexOfNew(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) + { + Span valueAsUtf16 = stackalloc char[Rune.MaxUtf16CharsPerRune]; + int charCount = value.EncodeToUtf16(valueAsUtf16); + return LastIndexOfNew(source, valueAsUtf16.Slice(0, charCount), options); + } + private static int LastIndexOfOrdinal(string source, string value, int startIndex, int count, bool ignoreCase) { Debug.Assert(!string.IsNullOrEmpty(source)); diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs b/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs index 3a703d661ff789..b2cbe18b02b902 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs @@ -20,6 +20,9 @@ namespace System.Text [DebuggerDisplay("{DebuggerDisplay,nq}")] public readonly struct Rune : IComparable, IEquatable { + internal const int MaxUtf16CharsPerRune = 2; // supplementary plane code points are encoded as 2 UTF-16 code units + internal const int MaxUtf8BytesPerRune = 4; // supplementary plane code points are encoded as 4 UTF-8 code units + private const char HighSurrogateStart = '\ud800'; private const char LowSurrogateStart = '\udc00'; private const int HighSurrogateRange = 0x3FF; @@ -163,7 +166,15 @@ private Rune(uint scalarValue, bool unused) /// /// The return value will be 1 or 2. /// - public int Utf16SequenceLength => UnicodeUtility.GetUtf16SequenceLength(_value); + public int Utf16SequenceLength + { + get + { + int codeUnitCount = UnicodeUtility.GetUtf16SequenceLength(_value); + Debug.Assert(codeUnitCount > 0 && codeUnitCount <= MaxUtf16CharsPerRune); + return codeUnitCount; + } + } /// /// Returns the length in code units of the @@ -172,7 +183,15 @@ private Rune(uint scalarValue, bool unused) /// /// The return value will be 1 through 4, inclusive. /// - public int Utf8SequenceLength => UnicodeUtility.GetUtf8SequenceLength(_value); + public int Utf8SequenceLength + { + get + { + int codeUnitCount = UnicodeUtility.GetUtf8SequenceLength(_value); + Debug.Assert(codeUnitCount > 0 && codeUnitCount <= MaxUtf8BytesPerRune); + return codeUnitCount; + } + } /// /// Returns the Unicode scalar value as an integer. @@ -185,8 +204,8 @@ private static Rune ChangeCaseCultureAware(Rune rune, TextInfo textInfo, bool to Debug.Assert(!GlobalizationMode.Invariant, "This should've been checked by the caller."); Debug.Assert(textInfo != null, "This should've been checked by the caller."); - Span original = stackalloc char[2]; // worst case scenario = 2 code units (for a surrogate pair) - Span modified = stackalloc char[2]; // case change should preserve UTF-16 code unit count + Span original = stackalloc char[MaxUtf16CharsPerRune]; + Span modified = stackalloc char[MaxUtf16CharsPerRune]; int charCount = rune.EncodeToUtf16(original); original = original.Slice(0, charCount); @@ -220,8 +239,8 @@ private static Rune ChangeCaseCultureAware(Rune rune, CultureInfo culture, bool Debug.Assert(!GlobalizationMode.Invariant, "This should've been checked by the caller."); Debug.Assert(culture != null, "This should've been checked by the caller."); - Span original = stackalloc char[2]; // worst case scenario = 2 code units (for a surrogate pair) - Span modified = stackalloc char[2]; // case change should preserve UTF-16 code unit count + Span original = stackalloc char[MaxUtf16CharsPerRune]; // worst case scenario = 2 code units (for a surrogate pair) + Span modified = stackalloc char[MaxUtf16CharsPerRune]; // case change should preserve UTF-16 code unit count int charCount = rune.EncodeToUtf16(original); original = original.Slice(0, charCount); @@ -885,7 +904,7 @@ public override string ToString() } else { - Span buffer = stackalloc char[2]; + Span buffer = stackalloc char[MaxUtf16CharsPerRune]; UnicodeUtility.GetUtf16SurrogatesFromSupplementaryPlaneScalar(_value, out buffer[0], out buffer[1]); return buffer.ToString(); } From 273a92ffd055e36f8b7c93534bde48a9bb8b41ac Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 10:53:24 -0700 Subject: [PATCH 03/30] Plumb MemoryExtensions through new CompareInfo public APIs --- .../src/System/Globalization/CompareInfo.cs | 77 +--------- .../System/MemoryExtensions.Globalization.cs | 131 ++++++------------ 2 files changed, 47 insertions(+), 161 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 93f8a11d0f8c05..23c76c7ea76213 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -368,27 +368,8 @@ internal int Compare(ReadOnlySpan string1, string? string2, CompareOptions return CompareString(string1, string2, options); } - internal int CompareOptionNone(ReadOnlySpan string1, ReadOnlySpan string2) - { - // Check for empty span or span from a null string - if (string1.Length == 0 || string2.Length == 0) - { - return string1.Length - string2.Length; - } - - return GlobalizationMode.Invariant ? - string.CompareOrdinal(string1, string2) : - CompareString(string1, string2, CompareOptions.None); - } - internal int CompareOptionIgnoreCase(ReadOnlySpan string1, ReadOnlySpan string2) { - // Check for empty span or span from a null string - if (string1.Length == 0 || string2.Length == 0) - { - return string1.Length - string2.Length; - } - return GlobalizationMode.Invariant ? CompareOrdinalIgnoreCase(string1, string2) : CompareString(string1, string2, CompareOptions.IgnoreCase); @@ -822,17 +803,6 @@ public bool IsPrefix(string source, string prefix, CompareOptions options) return StartsWith(source, prefix, options); } - internal bool IsPrefix(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options) - { - Debug.Assert(prefix.Length != 0); - Debug.Assert(source.Length != 0); - Debug.Assert((options & ValidIndexMaskOffFlags) == 0); - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); - - return StartsWith(source, prefix, options); - } - /// /// Determines whether a string starts with a specific prefix. /// @@ -956,17 +926,6 @@ public bool IsSuffix(string source, string suffix, CompareOptions options) return EndsWith(source, suffix, options); } - internal bool IsSuffix(ReadOnlySpan source, ReadOnlySpan suffix, CompareOptions options) - { - Debug.Assert(suffix.Length != 0); - Debug.Assert(source.Length != 0); - Debug.Assert((options & ValidIndexMaskOffFlags) == 0); - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); - - return EndsWith(source, suffix, options); - } - /// /// Determines whether a string ends with a specific suffix. /// @@ -1302,15 +1261,6 @@ public int IndexOfNew(ReadOnlySpan source, Rune value, CompareOptions opti return IndexOfNew(source, valueAsUtf16.Slice(0, charCount), options); } - internal int IndexOfOrdinalIgnoreCase(ReadOnlySpan source, ReadOnlySpan value) - { - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(!source.IsEmpty); - Debug.Assert(!value.IsEmpty); - - return IndexOfOrdinalCore(source, value, ignoreCase: true, fromBeginning: true); - } - internal static int IndexOfOrdinalIgnoreCaseNew(ReadOnlySpan source, ReadOnlySpan value, bool fromBeginning) { if (value.IsEmpty) @@ -1323,7 +1273,8 @@ internal static int IndexOfOrdinalIgnoreCaseNew(ReadOnlySpan source, ReadO if (value.Length > source.Length) { // A non-linguistic search compares chars directly against one another, so large - // target strings can never be found inside small search spaces. + // target strings can never be found inside small search spaces. This check also + // handles empty 'source' spans. return -1; } @@ -1338,30 +1289,6 @@ internal static int IndexOfOrdinalIgnoreCaseNew(ReadOnlySpan source, ReadO } } - internal int LastIndexOfOrdinal(ReadOnlySpan source, ReadOnlySpan value, bool ignoreCase) - { - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(!source.IsEmpty); - Debug.Assert(!value.IsEmpty); - return IndexOfOrdinalCore(source, value, ignoreCase, fromBeginning: false); - } - - internal unsafe int IndexOf(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options) - { - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(!source.IsEmpty); - Debug.Assert(!value.IsEmpty); - return IndexOfCore(source, value, options, null, fromBeginning: true); - } - - internal unsafe int LastIndexOf(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options) - { - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(!source.IsEmpty); - Debug.Assert(!value.IsEmpty); - return IndexOfCore(source, value, options, null, fromBeginning: false); - } - /// /// The following IndexOf overload is mainly used by String.Replace. This overload assumes the parameters are already validated /// and the caller is passing a valid matchLengthPtr pointer. diff --git a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs index a2f243b4b31c24..d498ef42f37a10 100644 --- a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs +++ b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs @@ -51,26 +51,20 @@ public static bool Equals(this ReadOnlySpan span, ReadOnlySpan other switch (comparisonType) { case StringComparison.CurrentCulture: - return CultureInfo.CurrentCulture.CompareInfo.CompareOptionNone(span, other) == 0; - case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.CompareOptionIgnoreCase(span, other) == 0; + return CultureInfo.CurrentCulture.CompareInfo.Compare(span, other, string.GetCaseCompareOfComparisonCulture(comparisonType)) == 0; case StringComparison.InvariantCulture: - return CompareInfo.Invariant.CompareOptionNone(span, other) == 0; - case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.CompareOptionIgnoreCase(span, other) == 0; + return CompareInfo.Invariant.Compare(span, other, string.GetCaseCompareOfComparisonCulture(comparisonType)) == 0; case StringComparison.Ordinal: return EqualsOrdinal(span, other); - case StringComparison.OrdinalIgnoreCase: + default: + Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); return EqualsOrdinalIgnoreCase(span, other); } - - Debug.Fail("StringComparison outside range"); - return false; } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -107,28 +101,22 @@ public static int CompareTo(this ReadOnlySpan span, ReadOnlySpan oth switch (comparisonType) { case StringComparison.CurrentCulture: - return CultureInfo.CurrentCulture.CompareInfo.CompareOptionNone(span, other); - case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.CompareOptionIgnoreCase(span, other); + return CultureInfo.CurrentCulture.CompareInfo.Compare(span, other, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: - return CompareInfo.Invariant.CompareOptionNone(span, other); - case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.CompareOptionIgnoreCase(span, other); + return CompareInfo.Invariant.Compare(span, other, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.Ordinal: if (span.Length == 0 || other.Length == 0) return span.Length - other.Length; return string.CompareOrdinal(span, other); - case StringComparison.OrdinalIgnoreCase: + default: + Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); return CompareInfo.CompareOrdinalIgnoreCase(span, other); } - - Debug.Fail("StringComparison outside range"); - return 0; } /// @@ -141,16 +129,6 @@ public static int IndexOf(this ReadOnlySpan span, ReadOnlySpan value { string.CheckStringComparison(comparisonType); - if (value.Length == 0) - { - return 0; // empty substring trivially occurs at every index (including start) of search space - } - - if (span.Length == 0) - { - return -1; - } - if (comparisonType == StringComparison.Ordinal) { return SpanHelpers.IndexOf( @@ -160,24 +138,19 @@ ref MemoryMarshal.GetReference(value), value.Length); } - if (GlobalizationMode.Invariant) - { - return CompareInfo.InvariantIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType) != CompareOptions.None); - } - switch (comparisonType) { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.IndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.IndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.IndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.IndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); default: Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); - return CompareInfo.Invariant.IndexOfOrdinalIgnoreCase(span, value); + return CompareInfo.IndexOfOrdinalIgnoreCaseNew(span, value, fromBeginning: true); } } @@ -191,34 +164,28 @@ public static int LastIndexOf(this ReadOnlySpan span, ReadOnlySpan v { string.CheckStringComparison(comparisonType); - if (value.Length == 0) - { - return span.Length; // empty substring trivially occurs at every index (including end) of search space - } - - if (span.Length == 0) - { - return -1; - } - - if (GlobalizationMode.Invariant) + if (comparisonType == StringComparison.Ordinal) { - return CompareInfo.InvariantIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType) != CompareOptions.None, fromBeginning: false); + return SpanHelpers.LastIndexOf( + ref MemoryMarshal.GetReference(span), + span.Length, + ref MemoryMarshal.GetReference(value), + value.Length); } switch (comparisonType) { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.LastIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.LastIndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.LastIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.LastIndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); default: - Debug.Assert(comparisonType == StringComparison.Ordinal || comparisonType == StringComparison.OrdinalIgnoreCase); - return CompareInfo.Invariant.LastIndexOfOrdinal(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType) != CompareOptions.None); + Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); + return CompareInfo.IndexOfOrdinalIgnoreCaseNew(span, value, fromBeginning: false); } } @@ -336,27 +303,23 @@ public static bool EndsWith(this ReadOnlySpan span, ReadOnlySpan val { string.CheckStringComparison(comparisonType); - if (value.Length == 0) + switch (comparisonType) { - return true; // the empty string is trivially a suffix of every other string - } + case StringComparison.CurrentCulture: + case StringComparison.CurrentCultureIgnoreCase: + return CultureInfo.CurrentCulture.CompareInfo.IsSuffixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); - if (comparisonType >= StringComparison.Ordinal || GlobalizationMode.Invariant) - { - if (string.GetCaseCompareOfComparisonCulture(comparisonType) == CompareOptions.None) - return span.EndsWith(value); + case StringComparison.InvariantCulture: + case StringComparison.InvariantCultureIgnoreCase: + return CompareInfo.Invariant.IsSuffixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); - return (span.Length >= value.Length) ? (CompareInfo.CompareOrdinalIgnoreCase(span.Slice(span.Length - value.Length), value) == 0) : false; - } + case StringComparison.Ordinal: + return span.EndsWith(value); - if (span.Length == 0) - { - return false; + default: + Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); + return span.EndsWithOrdinalIgnoreCase(value); } - - return (comparisonType >= StringComparison.InvariantCulture) ? - CompareInfo.Invariant.IsSuffix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)) : - CultureInfo.CurrentCulture.CompareInfo.IsSuffix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -379,27 +342,23 @@ public static bool StartsWith(this ReadOnlySpan span, ReadOnlySpan v { string.CheckStringComparison(comparisonType); - if (value.Length == 0) + switch (comparisonType) { - return true; // the empty string is trivially a prefix of every other string - } + case StringComparison.CurrentCulture: + case StringComparison.CurrentCultureIgnoreCase: + return CultureInfo.CurrentCulture.CompareInfo.IsPrefixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); - if (comparisonType >= StringComparison.Ordinal || GlobalizationMode.Invariant) - { - if (string.GetCaseCompareOfComparisonCulture(comparisonType) == CompareOptions.None) - return span.StartsWith(value); + case StringComparison.InvariantCulture: + case StringComparison.InvariantCultureIgnoreCase: + return CompareInfo.Invariant.IsPrefixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); - return (span.Length >= value.Length) ? (CompareInfo.CompareOrdinalIgnoreCase(span.Slice(0, value.Length), value) == 0) : false; - } + case StringComparison.Ordinal: + return span.StartsWith(value); - if (span.Length == 0) - { - return false; + default: + Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); + return span.StartsWithOrdinalIgnoreCase(value); } - - return (comparisonType >= StringComparison.InvariantCulture) ? - CompareInfo.Invariant.IsPrefix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)) : - CultureInfo.CurrentCulture.CompareInfo.IsPrefix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); } [MethodImpl(MethodImplOptions.AggressiveInlining)] From ff709a0b1958d578a1b50c068c14b5af4d4c9689 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 11:12:18 -0700 Subject: [PATCH 04/30] Cleanup CompareInfo.IsSortable, use mostly safe code --- .../System/Globalization/CompareInfo.Unix.cs | 38 ++++++------------- .../Globalization/CompareInfo.Windows.cs | 9 +++-- .../src/System/Globalization/CompareInfo.cs | 21 ++++------ 3 files changed, 25 insertions(+), 43 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs index d0db147a746070..f3ac23d31ae7a2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Runtime.InteropServices; +using System.Text; namespace System.Globalization { @@ -800,43 +801,28 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options) return new SortKey(this, source, options, keyData); } - private static unsafe bool IsSortable(char *text, int length) + private static bool IsSortableCore(ReadOnlySpan text) { Debug.Assert(!GlobalizationMode.Invariant); + Debug.Assert(!text.IsEmpty); - int index = 0; - UnicodeCategory uc; - - while (index < length) + do { - if (char.IsHighSurrogate(text[index])) - { - if (index == length - 1 || !char.IsLowSurrogate(text[index+1])) - return false; // unpaired surrogate - - uc = CharUnicodeInfo.GetUnicodeCategory(char.ConvertToUtf32(text[index], text[index+1])); - if (uc == UnicodeCategory.PrivateUse || uc == UnicodeCategory.OtherNotAssigned) - return false; - - index += 2; - continue; - } - - if (char.IsLowSurrogate(text[index])) + if (Rune.DecodeFromUtf16(text, out Rune result, out int charsConsumed) != OperationStatus.Done) { - return false; // unpaired surrogate + return false; // found an unpaired surrogate somewhere in the text } - uc = CharUnicodeInfo.GetUnicodeCategory(text[index]); - if (uc == UnicodeCategory.PrivateUse || uc == UnicodeCategory.OtherNotAssigned) + UnicodeCategory category = Rune.GetUnicodeCategory(result); + if (category == UnicodeCategory.PrivateUse || category == UnicodeCategory.OtherNotAssigned) { - return false; + return false; // can't sort private use or unassigned code points } - index++; - } + text = text.Slice(charsConsumed); + } while (!text.IsEmpty); - return true; + return true; // saw no unsortable data in the buffer } // ----------------------------- diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index 16a29590c2cd60..a7502e9d76bd65 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -624,12 +624,15 @@ private unsafe int GetSortKeyLengthCore(ReadOnlySpan source, CompareOption return sortKeyLength; } - private static unsafe bool IsSortable(char* text, int length) + private static unsafe bool IsSortableCore(ReadOnlySpan text) { Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(text != null); + Debug.Assert(!text.IsEmpty); - return Interop.Kernel32.IsNLSDefinedString(Interop.Kernel32.COMPARE_STRING, 0, IntPtr.Zero, text, length); + fixed (char* pText = &MemoryMarshal.GetReference(text)) + { + return Interop.Kernel32.IsNLSDefinedString(Interop.Kernel32.COMPARE_STRING, 0, IntPtr.Zero, pText, text.Length); + } } private const int COMPARE_OPTIONS_ORDINAL = 0x40000000; // Ordinal diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 23c76c7ea76213..afd161c237e09c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -124,18 +124,17 @@ public static CompareInfo GetCompareInfo(string name) return CultureInfo.GetCultureInfo(name).CompareInfo; } - public static unsafe bool IsSortable(char ch) + public static bool IsSortable(char ch) { if (GlobalizationMode.Invariant) { - return true; + return true; // all chars are sortable in invariant mode } - char* pChar = &ch; - return IsSortable(pChar, 1); + return IsSortableCore(MemoryMarshal.CreateReadOnlySpan(ref ch, 1)); } - public static unsafe bool IsSortable(string text) + public static bool IsSortable(string text) { if (text == null) { @@ -152,10 +151,7 @@ public static unsafe bool IsSortable(string text) return true; } - fixed (char* pChar = text) - { - return IsSortable(pChar, text.Length); - } + return IsSortableCore(text); } /// @@ -166,7 +162,7 @@ public static unsafe bool IsSortable(string text) /// if is non-empty and contains /// only sortable Unicode characters; otherwise, . /// - public static unsafe bool IsSortable(ReadOnlySpan text) + public static bool IsSortable(ReadOnlySpan text) { if (text.Length == 0) { @@ -178,10 +174,7 @@ public static unsafe bool IsSortable(ReadOnlySpan text) return true; } - fixed (char* pChar = text) - { - return IsSortable(pChar, text.Length); - } + return IsSortableCore(text); } /// From 4e687672dcc5d7016353288c8e7df3b22f3e754d Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 11:17:42 -0700 Subject: [PATCH 05/30] Plumb CompareInfo.IsPrefix/IsSuffix through span-based APIs --- .../src/System/Globalization/CompareInfo.cs | 74 ++----------------- .../src/System/ThrowHelper.cs | 6 ++ 2 files changed, 14 insertions(+), 66 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index afd161c237e09c..610711a78ed85c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -757,43 +757,14 @@ public bool IsPrefix(string source, string prefix, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } if (prefix == null) { - throw new ArgumentNullException(nameof(prefix)); - } - - if (prefix.Length == 0) - { - return true; - } - if (source.Length == 0) - { - return false; - } - - if (options == CompareOptions.OrdinalIgnoreCase) - { - return source.StartsWith(prefix, StringComparison.OrdinalIgnoreCase); - } - - if (options == CompareOptions.Ordinal) - { - return source.StartsWith(prefix, StringComparison.Ordinal); - } - - if ((options & ValidIndexMaskOffFlags) != 0) - { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); - } - - if (GlobalizationMode.Invariant) - { - return source.StartsWith(prefix, (options & CompareOptions.IgnoreCase) != 0 ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.prefix); } - return StartsWith(source, prefix, options); + return IsPrefixNew(source, prefix, options); } /// @@ -869,7 +840,7 @@ public bool IsPrefixNew(ReadOnlySpan source, ReadOnlySpan prefix, Co public bool IsPrefix(string source, string prefix) { - return IsPrefix(source, prefix, 0); + return IsPrefix(source, prefix, CompareOptions.None); } /// @@ -880,43 +851,14 @@ public bool IsSuffix(string source, string suffix, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } if (suffix == null) { - throw new ArgumentNullException(nameof(suffix)); - } - - if (suffix.Length == 0) - { - return true; - } - if (source.Length == 0) - { - return false; - } - - if (options == CompareOptions.OrdinalIgnoreCase) - { - return source.EndsWith(suffix, StringComparison.OrdinalIgnoreCase); - } - - if (options == CompareOptions.Ordinal) - { - return source.EndsWith(suffix, StringComparison.Ordinal); - } - - if ((options & ValidIndexMaskOffFlags) != 0) - { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); - } - - if (GlobalizationMode.Invariant) - { - return source.EndsWith(suffix, (options & CompareOptions.IgnoreCase) != 0 ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.suffix); } - return EndsWith(source, suffix, options); + return IsSuffixNew(source, suffix, options); } /// @@ -992,7 +934,7 @@ public bool IsSuffixNew(ReadOnlySpan source, ReadOnlySpan suffix, Co public bool IsSuffix(string source, string suffix) { - return IsSuffix(source, suffix, 0); + return IsSuffix(source, suffix, CompareOptions.None); } /// diff --git a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs index 2d776129fbed76..e4516e6008cdb8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs @@ -690,6 +690,10 @@ private static string GetArgumentName(ExceptionArgument argument) return "str"; case ExceptionArgument.options: return "options"; + case ExceptionArgument.prefix: + return "prefix"; + case ExceptionArgument.suffix: + return "suffix"; default: Debug.Fail("The enum value is not defined, please check the ExceptionArgument Enum."); return ""; @@ -944,6 +948,8 @@ internal enum ExceptionArgument codePoint, str, options, + prefix, + suffix, } // From 7c4bc13bedfa33439d5e989152f396e07e787c5e Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 11:46:08 -0700 Subject: [PATCH 06/30] Plumb CompareInfo.IndexOf atop span-based APIs --- .../src/System/Globalization/CompareInfo.cs | 125 +++++++----------- .../src/System/String.cs | 22 +++ 2 files changed, 72 insertions(+), 75 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 610711a78ed85c..2d206a180c5539 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -943,79 +943,68 @@ public bool IsSuffix(string source, string suffix) /// the specified value is not found. If value equals string.Empty, /// startIndex is returned. Throws IndexOutOfRange if startIndex or /// endIndex is less than zero or greater than the length of string. - /// Throws ArgumentException if value is null. + /// Throws ArgumentException if value (as a string) is null. /// public int IndexOf(string source, char value) { - if (source == null) - { - throw new ArgumentNullException(nameof(source)); - } - - return IndexOf(source, value, 0, source.Length, CompareOptions.None); + return IndexOf(source, value, CompareOptions.None); } public int IndexOf(string source, string value) { - if (source == null) - throw new ArgumentNullException(nameof(source)); - - return IndexOf(source, value, 0, source.Length, CompareOptions.None); + return IndexOf(source, value, CompareOptions.None); } public int IndexOf(string source, char value, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - return IndexOf(source, value, 0, source.Length, options); + return IndexOfNew(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); } public int IndexOf(string source, string value, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); + } + if (value == null) + { + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - return IndexOf(source, value, 0, source.Length, options); + return IndexOfNew(source, value, options); } public int IndexOf(string source, char value, int startIndex) { - if (source == null) - { - throw new ArgumentNullException(nameof(source)); - } - - return IndexOf(source, value, startIndex, source.Length - startIndex, CompareOptions.None); + return IndexOf(source, value, startIndex, CompareOptions.None); } public int IndexOf(string source, string value, int startIndex) { - if (source == null) - throw new ArgumentNullException(nameof(source)); - - return IndexOf(source, value, startIndex, source.Length - startIndex, CompareOptions.None); + return IndexOf(source, value, startIndex, CompareOptions.None); } public int IndexOf(string source, char value, int startIndex, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } return IndexOf(source, value, startIndex, source.Length - startIndex, options); + } public int IndexOf(string source, string value, int startIndex, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } return IndexOf(source, value, startIndex, source.Length - startIndex, options); @@ -1035,76 +1024,64 @@ public unsafe int IndexOf(string source, char value, int startIndex, int count, { if (source == null) { - throw new ArgumentNullException(nameof(source)); - } - if (startIndex < 0 || startIndex > source.Length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - } - if (count < 0 || startIndex > source.Length - count) - { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - if (source.Length == 0) + if (!source.TryGetSpan(startIndex, count, out ReadOnlySpan sourceSpan)) { - return -1; + // Bounds check failed - figure out exactly what went wrong so that we can + // surface the correct argument exception. + + if ((uint)startIndex > (uint)source.Length) + { + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_Index); + } + else + { + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count, ExceptionResource.ArgumentOutOfRange_Count); + } } - // Validate CompareOptions - // Ordinal can't be selected with other flags - if ((options & ValidIndexMaskOffFlags) != 0 && (options != CompareOptions.Ordinal && options != CompareOptions.OrdinalIgnoreCase)) + int result = IndexOfNew(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + if (result >= 0) { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + result += startIndex; } - - return IndexOf(source, char.ToString(value), startIndex, count, options, null); + return result; } public unsafe int IndexOf(string source, string value, int startIndex, int count, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } if (value == null) { - throw new ArgumentNullException(nameof(value)); - } - if (startIndex > source.Length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - // In Everett we used to return -1 for empty string even if startIndex is negative number so we keeping same behavior here. - // We return 0 if both source and value are empty strings for Everett compatibility too. - if (source.Length == 0) + if (!source.TryGetSpan(startIndex, count, out ReadOnlySpan sourceSpan)) { - if (value.Length == 0) + // Bounds check failed - figure out exactly what went wrong so that we can + // surface the correct argument exception. + + if ((uint)startIndex > (uint)source.Length) { - return 0; + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_Index); + } + else + { + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count, ExceptionResource.ArgumentOutOfRange_Count); } - return -1; - } - - if (startIndex < 0) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - } - - if (count < 0 || startIndex > source.Length - count) - { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); } - // Validate CompareOptions - // Ordinal can't be selected with other flags - if ((options & ValidIndexMaskOffFlags) != 0 && (options != CompareOptions.Ordinal && options != CompareOptions.OrdinalIgnoreCase)) + int result = IndexOfNew(sourceSpan, value, options); + if (result >= 0) { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + result += startIndex; } - - return IndexOf(source, value, startIndex, count, options, null); + return result; } /// @@ -1163,9 +1140,7 @@ public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value } else { - throw new ArgumentException( - paramName: nameof(options), - message: SR.Argument_InvalidFlag); + ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidFlag, ExceptionArgument.options); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index f29d327a260d44..00582d5ebca437 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -378,6 +378,28 @@ public static string Create(int length, TState state, SpanAction(string? value) => value != null ? new ReadOnlySpan(ref value.GetRawStringData(), value.Length) : default; + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal bool TryGetSpan(int startIndex, int count, out ReadOnlySpan slice) + { +#if TARGET_64BIT + // See comment in Span.Slice for how this works. + if ((ulong)(uint)startIndex + (ulong)(uint)count > (ulong)(uint)Length) + { + slice = default; + return false; + } +#else + if ((uint)startIndex > (uint)Length || (uint)count > (uint)(Length - startIndex)) + { + slice = default; + return false; + } +#endif + + slice = new ReadOnlySpan(ref Unsafe.Add(ref _firstChar, startIndex), count); + return true; + } + public object Clone() { return this; From 521e7358ae30d7e937fa9a57e5febccbe2a9366e Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 16:08:55 -0700 Subject: [PATCH 07/30] Rewrite CompareInfo.Compare in terms of Span --- .../src/System/Globalization/CompareInfo.cs | 189 ++++++++++-------- 1 file changed, 105 insertions(+), 84 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 2d206a180c5539..f1f601d0134cf8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -270,53 +271,31 @@ public int Compare(string? string1, string? string2) public int Compare(string? string1, string? string2, CompareOptions options) { - if (options == CompareOptions.OrdinalIgnoreCase) - { - return string.Compare(string1, string2, StringComparison.OrdinalIgnoreCase); - } - - // Verify the options before we do any real comparison. - if ((options & CompareOptions.Ordinal) != 0) - { - if (options != CompareOptions.Ordinal) - { - throw new ArgumentException(SR.Argument_CompareOptionOrdinal, nameof(options)); - } - - return string.CompareOrdinal(string1, string2); - } - - if ((options & ValidCompareMaskOffFlags) != 0) - { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); - } + int retVal; // Our paradigm is that null sorts less than any other string and // that two nulls sort as equal. + if (string1 == null) { - if (string2 == null) - { - return 0; - } - return -1; // null < non-null + retVal = (string2 == null) ? 0 : -1; + goto CheckOptionsAndReturn; } if (string2 == null) { - return 1; // non-null > null + retVal = 1; + goto CheckOptionsAndReturn; } - if (GlobalizationMode.Invariant) - { - if ((options & CompareOptions.IgnoreCase) != 0) - { - return CompareOrdinalIgnoreCase(string1, string2); - } + return Compare(string1.AsSpan(), string2.AsSpan(), options); - return string.CompareOrdinal(string1, string2); - } + CheckOptionsAndReturn: + + // If we're short-circuiting the globalization logic, we still need to check that + // the provided options were valid. - return CompareString(string1.AsSpan(), string2.AsSpan(), options); + CheckCompareOptionsForCompare(options); + return retVal; } // TODO https://github.com/dotnet/runtime/issues/8890: @@ -377,7 +356,7 @@ internal int CompareOptionIgnoreCase(ReadOnlySpan string1, ReadOnlySpan public int Compare(string? string1, int offset1, int length1, string? string2, int offset2, int length2) { - return Compare(string1, offset1, length1, string2, offset2, length2, 0); + return Compare(string1, offset1, length1, string2, offset2, length2, CompareOptions.None); } public int Compare(string? string1, int offset1, string? string2, int offset2, CompareOptions options) @@ -388,83 +367,94 @@ public int Compare(string? string1, int offset1, string? string2, int offset2, C public int Compare(string? string1, int offset1, string? string2, int offset2) { - return Compare(string1, offset1, string2, offset2, 0); + return Compare(string1, offset1, string2, offset2, CompareOptions.None); } public int Compare(string? string1, int offset1, int length1, string? string2, int offset2, int length2, CompareOptions options) { - if (options == CompareOptions.OrdinalIgnoreCase) + ReadOnlySpan span1 = default; + ReadOnlySpan span2 = default; + + if (string1 == null) { - int result = string.Compare(string1, offset1, string2, offset2, length1 < length2 ? length1 : length2, StringComparison.OrdinalIgnoreCase); - if ((length1 != length2) && result == 0) + if (offset1 != 0 || length1 != 0) { - return length1 > length2 ? 1 : -1; + goto BoundsCheckError; } - - return result; - } - - if (length1 < 0 || length2 < 0) - { - throw new ArgumentOutOfRangeException((length1 < 0) ? nameof(length1) : nameof(length2), SR.ArgumentOutOfRange_NeedPosNum); } - if (offset1 < 0 || offset2 < 0) - { - throw new ArgumentOutOfRangeException((offset1 < 0) ? nameof(offset1) : nameof(offset2), SR.ArgumentOutOfRange_NeedPosNum); - } - if (offset1 > (string1 == null ? 0 : string1.Length) - length1) - { - throw new ArgumentOutOfRangeException(nameof(string1), SR.ArgumentOutOfRange_OffsetLength); - } - if (offset2 > (string2 == null ? 0 : string2.Length) - length2) + else if (!string1.TryGetSpan(offset1, length1, out span1)) { - throw new ArgumentOutOfRangeException(nameof(string2), SR.ArgumentOutOfRange_OffsetLength); + goto BoundsCheckError; } - if ((options & CompareOptions.Ordinal) != 0) + + if (string2 == null) { - if (options != CompareOptions.Ordinal) + if (offset2 != 0 || length2 != 0) { - throw new ArgumentException(SR.Argument_CompareOptionOrdinal, - nameof(options)); + goto BoundsCheckError; } } - else if ((options & ValidCompareMaskOffFlags) != 0) + else if (!string2.TryGetSpan(offset2, length2, out span2)) { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + goto BoundsCheckError; } + // At this point both string1 and string2 have been bounds-checked. + + int retVal; + + // Our paradigm is that null sorts less than any other string and + // that two nulls sort as equal. + if (string1 == null) { - if (string2 == null) - { - return 0; - } - return -1; + retVal = (string2 == null) ? 0 : -1; + goto CheckOptionsAndReturn; } if (string2 == null) { - return 1; + retVal = 1; + goto CheckOptionsAndReturn; } - ReadOnlySpan span1 = string1.AsSpan(offset1, length1); - ReadOnlySpan span2 = string2.AsSpan(offset2, length2); + // At this point we know both string1 and string2 weren't null, + // though they may have been empty. - if (options == CompareOptions.Ordinal) + Debug.Assert(!Unsafe.IsNullRef(ref MemoryMarshal.GetReference(span1))); + Debug.Assert(!Unsafe.IsNullRef(ref MemoryMarshal.GetReference(span2))); + + return Compare(span1, span2, options); + + CheckOptionsAndReturn: + + // If we're short-circuiting the globalization logic, we still need to check that + // the provided options were valid. + + CheckCompareOptionsForCompare(options); + return retVal; + + BoundsCheckError: + + // We know a bounds check error occurred. Now we just need to figure + // out the correct error message to surface. + + if (length1 < 0 || length2 < 0) { - return string.CompareOrdinal(span1, span2); + throw new ArgumentOutOfRangeException((length1 < 0) ? nameof(length1) : nameof(length2), SR.ArgumentOutOfRange_NeedPosNum); } - if (GlobalizationMode.Invariant) + if (offset1 < 0 || offset2 < 0) { - if ((options & CompareOptions.IgnoreCase) != 0) - { - return CompareOrdinalIgnoreCase(span1, span2); - } + throw new ArgumentOutOfRangeException((offset1 < 0) ? nameof(offset1) : nameof(offset2), SR.ArgumentOutOfRange_NeedPosNum); + } - return string.CompareOrdinal(span1, span2); + if (offset1 > (string1 == null ? 0 : string1.Length) - length1) + { + throw new ArgumentOutOfRangeException(nameof(string1), SR.ArgumentOutOfRange_OffsetLength); } - return CompareString(span1, span2, options); + Debug.Assert(offset2 > (string2 == null ? 0 : string2.Length) - length2); + throw new ArgumentOutOfRangeException(nameof(string2), SR.ArgumentOutOfRange_OffsetLength); } /// @@ -483,6 +473,12 @@ public int Compare(string? string1, int offset1, int length1, string? string2, i /// public int Compare(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options = CompareOptions.None) { + if (string1 == string2) // referential equality + length + { + CheckCompareOptionsForCompare(options); + return 0; + } + if ((options & ValidCompareMaskOffFlags) == 0) { // Common case: caller is attempting to perform linguistic comparison. @@ -517,9 +513,7 @@ public int Compare(ReadOnlySpan string1, ReadOnlySpan string2, Compa } else { - throw new ArgumentException( - paramName: nameof(options), - message: SR.Argument_InvalidFlag); + ThrowCompareOptionsCheckFailed(options); } } @@ -530,6 +524,33 @@ public int Compare(ReadOnlySpan string1, ReadOnlySpan string2, Compa return CompareOrdinalIgnoreCase(string1, string2); } + // Checks that 'CompareOptions' is valid for a call to Compare, throwing the appropriate + // exception if the check fails. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + [StackTraceHidden] + private static void CheckCompareOptionsForCompare(CompareOptions options) + { + // Any combination of defined CompareOptions flags is valid, except for + // Ordinal and OrdinalIgnoreCase, which may only be used in isolation. + + if ((options & ValidCompareMaskOffFlags) != 0) + { + if (options != CompareOptions.Ordinal && options != CompareOptions.OrdinalIgnoreCase) + { + ThrowCompareOptionsCheckFailed(options); + } + } + } + + [DoesNotReturn] + [StackTraceHidden] + private static void ThrowCompareOptionsCheckFailed(CompareOptions options) + { + throw new ArgumentException( + paramName: nameof(options), + message: ((options & CompareOptions.Ordinal) != 0) ? SR.Argument_CompareOptionOrdinal : SR.Argument_InvalidFlag); + } + /// /// CompareOrdinalIgnoreCase compare two string ordinally with ignoring the case. /// it assumes the strings are Ascii string till we hit non Ascii character in strA or strB and then we continue the comparison by From 25bd0d83029e6a1a966cd49fa50394f75503e24f Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 16:17:18 -0700 Subject: [PATCH 08/30] Plumb CompareInfo.GetHashCode through Span versions --- .../src/System/Globalization/CompareInfo.cs | 87 +++++++++---------- 1 file changed, 39 insertions(+), 48 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index f1f601d0134cf8..00d232827ffb01 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -846,9 +846,7 @@ public bool IsPrefixNew(ReadOnlySpan source, ReadOnlySpan prefix, Co } else { - throw new ArgumentException( - paramName: nameof(options), - message: SR.Argument_InvalidFlag); + ThrowCompareOptionsCheckFailed(options); } } @@ -940,9 +938,7 @@ public bool IsSuffixNew(ReadOnlySpan source, ReadOnlySpan suffix, Co } else { - throw new ArgumentException( - paramName: nameof(options), - message: SR.Argument_InvalidFlag); + ThrowCompareOptionsCheckFailed(options); } } @@ -1804,62 +1800,57 @@ public int GetHashCode(string source, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - if ((options & ValidCompareMaskOffFlags) == 0) - { - // No unsupported flags are set - continue on with the regular logic - if (GlobalizationMode.Invariant) - { - return ((options & CompareOptions.IgnoreCase) != 0) ? source.GetHashCodeOrdinalIgnoreCase() : source.GetHashCode(); - } - return GetHashCodeOfStringCore(source, options); - } - else if (options == CompareOptions.Ordinal) - { - // We allow Ordinal in isolation - return source.GetHashCode(); - } - else if (options == CompareOptions.OrdinalIgnoreCase) - { - // We allow OrdinalIgnoreCase in isolation - return source.GetHashCodeOrdinalIgnoreCase(); - } - else - { - // Unsupported combination of flags specified - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); - } + return GetHashCode(source.AsSpan(), options); } public int GetHashCode(ReadOnlySpan source, CompareOptions options) { if ((options & ValidCompareMaskOffFlags) == 0) { - // No unsupported flags are set - continue on with the regular logic - if (GlobalizationMode.Invariant) + // Common case: caller is attempting to get a linguistic sort key. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. + + if (!GlobalizationMode.Invariant) { - return ((options & CompareOptions.IgnoreCase) != 0) ? string.GetHashCodeOrdinalIgnoreCase(source) : string.GetHashCode(source); + return GetHashCodeOfStringCore(source, options); + } + else if ((options & CompareOptions.IgnoreCase) == 0) + { + goto ReturnOrdinal; + } + else + { + goto ReturnOrdinalIgnoreCase; } - - return GetHashCodeOfStringCore(source, options); - } - else if (options == CompareOptions.Ordinal) - { - // We allow Ordinal in isolation - return string.GetHashCode(source); - } - else if (options == CompareOptions.OrdinalIgnoreCase) - { - // We allow OrdinalIgnoreCase in isolation - return string.GetHashCodeOrdinalIgnoreCase(source); } else { - // Unsupported combination of flags specified - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + // Less common case: caller is attempting to get a non-linguistic sort key, + // or an invalid combination of flags was supplied. + + if (options == CompareOptions.Ordinal) + { + goto ReturnOrdinal; + } + else if (options == CompareOptions.OrdinalIgnoreCase) + { + goto ReturnOrdinalIgnoreCase; + } + else + { + ThrowCompareOptionsCheckFailed(options); + } } + + ReturnOrdinal: + return string.GetHashCode(source); + + ReturnOrdinalIgnoreCase: + return string.GetHashCodeOrdinalIgnoreCase(source); } public override string ToString() => "CompareInfo - " + Name; From 4d1d3f06065c0804270e26ed7fcc711b78408c43 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 8 Apr 2020 16:41:04 -0700 Subject: [PATCH 09/30] Plumb CompareInfo.LastIndexOf through the Span APIs --- .../System/Globalization/CompareInfo.Unix.cs | 46 ----- .../Globalization/CompareInfo.Windows.cs | 31 ---- .../src/System/Globalization/CompareInfo.cs | 174 ++++++++---------- 3 files changed, 80 insertions(+), 171 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs index f3ac23d31ae7a2..f809db19702177 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs @@ -458,52 +458,6 @@ private unsafe int IndexOfOrdinalHelper(ReadOnlySpan source, ReadOnlySpan< } } - private unsafe int LastIndexOfCore(string source, string target, int startIndex, int count, CompareOptions options) - { - Debug.Assert(!GlobalizationMode.Invariant); - - Debug.Assert(!string.IsNullOrEmpty(source)); - Debug.Assert(target != null); - Debug.Assert((options & CompareOptions.OrdinalIgnoreCase) == 0); - - // startIndex points to the final char to include in the search space. - // empty target strings trivially occur at the end of the search space. - - if (target.Length == 0) - { - return startIndex + 1; - } - - if (options == CompareOptions.Ordinal) - { - return LastIndexOfOrdinalCore(source, target, startIndex, count, ignoreCase: false); - } - - // startIndex is the index into source where we start search backwards from. leftStartIndex is the index into source - // of the start of the string that is count characters away from startIndex. - int leftStartIndex = (startIndex - count + 1); - - int lastIndex; - - if (_isAsciiEqualityOrdinal && CanUseAsciiOrdinalForOptions(options)) - { - if ((options & CompareOptions.IgnoreCase) != 0) - lastIndex = IndexOfOrdinalIgnoreCaseHelper(source.AsSpan(leftStartIndex, count), target.AsSpan(), options, matchLengthPtr: null, fromBeginning: false); - else - lastIndex = IndexOfOrdinalHelper(source.AsSpan(leftStartIndex, count), target.AsSpan(), options, matchLengthPtr: null, fromBeginning: false); - } - else - { - fixed (char* pSource = source) - fixed (char* pTarget = target) - { - lastIndex = Interop.Globalization.LastIndexOf(_sortHandle, pTarget, target.Length, pSource + (startIndex - count + 1), count, options); - } - } - - return lastIndex != -1 ? lastIndex + leftStartIndex : -1; - } - private unsafe bool StartsWith(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index a7502e9d76bd65..bc814e4a399721 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -365,37 +365,6 @@ internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan ta return FindString(positionFlag | (uint)GetNativeCompareFlags(options), source, target, matchLengthPtr); } - private unsafe int LastIndexOfCore(string source, string target, int startIndex, int count, CompareOptions options) - { - Debug.Assert(!GlobalizationMode.Invariant); - - Debug.Assert(!string.IsNullOrEmpty(source)); - Debug.Assert(target != null); - Debug.Assert((options & CompareOptions.OrdinalIgnoreCase) == 0); - - // startIndex points to the final char to include in the search space. - // empty target strings trivially occur at the end of the search space. - - if (target.Length == 0) - return startIndex + 1; - - if ((options & CompareOptions.Ordinal) != 0) - { - return FastLastIndexOfString(source, target, startIndex, count, target.Length); - } - else - { - int retValue = FindString(FIND_FROMEND | (uint)GetNativeCompareFlags(options), source.AsSpan(startIndex - count + 1, count), target, null); - - if (retValue >= 0) - { - return retValue + startIndex - (count - 1); - } - } - - return -1; - } - private unsafe bool StartsWith(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 00d232827ffb01..e459a55f21f551 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -1381,51 +1381,40 @@ ref value.GetRawStringData(), /// the specified value is not found. If value equals string.Empty, /// endIndex is returned. Throws IndexOutOfRange if startIndex or /// endIndex is less than zero or greater than the length of string. - /// Throws ArgumentException if value is null. + /// Throws ArgumentException if value (as a string) is null. /// public int LastIndexOf(string source, char value) { - if (source == null) - { - throw new ArgumentNullException(nameof(source)); - } - - // Can't start at negative index, so make sure we check for the length == 0 case. - return LastIndexOf(source, value, source.Length - 1, source.Length, CompareOptions.None); + return LastIndexOf(source, value, CompareOptions.None); } public int LastIndexOf(string source, string value) { - if (source == null) - { - throw new ArgumentNullException(nameof(source)); - } - - // Can't start at negative index, so make sure we check for the length == 0 case. - return LastIndexOf(source, value, source.Length - 1, - source.Length, CompareOptions.None); + return LastIndexOf(source, value, CompareOptions.None); } public int LastIndexOf(string source, char value, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - // Can't start at negative index, so make sure we check for the length == 0 case. - return LastIndexOf(source, value, source.Length - 1, source.Length, options); + return LastIndexOfNew(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); } public int LastIndexOf(string source, string value, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); + } + if (value == null) + { + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - // Can't start at negative index, so make sure we check for the length == 0 case. - return LastIndexOf(source, value, source.Length - 1, source.Length, options); + return LastIndexOfNew(source, value, options); } public int LastIndexOf(string source, char value, int startIndex) @@ -1462,119 +1451,116 @@ public int LastIndexOf(string source, char value, int startIndex, int count, Com { if (source == null) { - throw new ArgumentNullException(nameof(source)); - } - // Validate CompareOptions - // Ordinal can't be selected with other flags - if ((options & ValidIndexMaskOffFlags) != 0 && - (options != CompareOptions.Ordinal) && - (options != CompareOptions.OrdinalIgnoreCase)) - { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - // Special case for 0 length input strings - if (source.Length == 0 && (startIndex == -1 || startIndex == 0)) - { - return -1; - } + TryAgain: - // Make sure we're not out of range - if (startIndex < 0 || startIndex > source.Length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - } + // Previous versions of the Framework special-cased empty 'source' to allow startIndex = -1 or startIndex = 0, + // ignoring 'count' and short-circuiting the entire operation. We'll silently fix up the 'count' parameter + // if this occurs. + // + // See the comments just before string.IndexOf(string) for more information on how these computations are + // performed. - // Make sure that we allow startIndex == source.Length - if (startIndex == source.Length) + if ((uint)startIndex >= (uint)source.Length) { - startIndex--; - if (count > 0) + if (startIndex == -1 && source.Length == 0) { - count--; + count = 0; // normalize } - } + else if (startIndex == source.Length) + { + // The caller likely had an off-by-one error when invoking the API. The Framework has historically + // allowed for this and tried to fix up the parameters, so we'll continue to do so for compat. - // 2nd have of this also catches when startIndex == MAXINT, so MAXINT - 0 + 1 == -1, which is < 0. - if (count < 0 || startIndex - count + 1 < 0) - { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); + startIndex--; + if (count > 0) + { + count--; + } + + goto TryAgain; // guaranteed never to loop more than once + } + else + { + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_Index); + } } - if (options == CompareOptions.OrdinalIgnoreCase) + startIndex = startIndex - count + 1; // this will be the actual index where we begin our search + + if (!source.TryGetSpan(startIndex, count, out ReadOnlySpan sourceSpan)) { - return source.LastIndexOf(value.ToString(), startIndex, count, StringComparison.OrdinalIgnoreCase); + ThrowHelper.ThrowCountArgumentOutOfRange_ArgumentOutOfRange_Count(); } - if (GlobalizationMode.Invariant) + int retVal = LastIndexOfNew(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + if (retVal >= 0) { - return InvariantLastIndexOf(source, char.ToString(value), startIndex, count, (options & (CompareOptions.IgnoreCase | CompareOptions.OrdinalIgnoreCase)) != 0); + retVal += startIndex; } - - return LastIndexOfCore(source, value.ToString(), startIndex, count, options); + return retVal; } public int LastIndexOf(string source, string value, int startIndex, int count, CompareOptions options) { if (source == null) { - throw new ArgumentNullException(nameof(source)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } if (value == null) { - throw new ArgumentNullException(nameof(value)); + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - // Validate CompareOptions - // Ordinal can't be selected with other flags - if ((options & ValidIndexMaskOffFlags) != 0 && - (options != CompareOptions.Ordinal) && - (options != CompareOptions.OrdinalIgnoreCase)) - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); + TryAgain: - // Special case for 0 length input strings - if (source.Length == 0 && (startIndex == -1 || startIndex == 0)) - { - return (value.Length == 0) ? 0 : -1; - } + // Previous versions of the Framework special-cased empty 'source' to allow startIndex = -1 or startIndex = 0, + // ignoring 'count' and short-circuiting the entire operation. We'll silently fix up the 'count' parameter + // if this occurs. + // + // See the comments just before string.IndexOf(string) for more information on how these computations are + // performed. - // Make sure we're not out of range - if (startIndex < 0 || startIndex > source.Length) + if ((uint)startIndex >= (uint)source.Length) { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - } - - // Make sure that we allow startIndex == source.Length - if (startIndex == source.Length) - { - startIndex--; - if (count > 0) + if (startIndex == -1 && source.Length == 0) { - count--; + count = 0; // normalize } + else if (startIndex == source.Length) + { + // The caller likely had an off-by-one error when invoking the API. The Framework has historically + // allowed for this and tried to fix up the parameters, so we'll continue to do so for compat. + + startIndex--; + if (count > 0) + { + count--; + } - // empty substrings trivially occur at the end of the search space - if (value.Length == 0 && count >= 0 && startIndex - count + 1 >= 0) + goto TryAgain; // guaranteed never to loop more than once + } + else { - return startIndex + 1; + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_Index); } } - // 2nd half of this also catches when startIndex == MAXINT, so MAXINT - 0 + 1 == -1, which is < 0. - if (count < 0 || startIndex - count + 1 < 0) + startIndex = startIndex - count + 1; // this will be the actual index where we begin our search + + if (!source.TryGetSpan(startIndex, count, out ReadOnlySpan sourceSpan)) { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); + ThrowHelper.ThrowCountArgumentOutOfRange_ArgumentOutOfRange_Count(); } - if (options == CompareOptions.OrdinalIgnoreCase) + int retVal = LastIndexOfNew(sourceSpan, value, options); + if (retVal >= 0) { - return LastIndexOfOrdinal(source, value, startIndex, count, ignoreCase: true); + retVal += startIndex; } - - if (GlobalizationMode.Invariant) - return InvariantLastIndexOf(source, value, startIndex, count, (options & (CompareOptions.IgnoreCase | CompareOptions.OrdinalIgnoreCase)) != 0); - - return LastIndexOfCore(source, value, startIndex, count, options); + return retVal; } /// From 555566d37a03d86fed18e62594a1b961189872b7 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 10:50:15 -0700 Subject: [PATCH 10/30] Plumb string.Replace through spanified code paths --- .../src/System/Globalization/CompareInfo.cs | 122 ++++++------------ .../src/System/String.Manipulation.cs | 91 +++++++------ .../src/System/Text/Utf8Span.Searching.cs | 2 +- 3 files changed, 97 insertions(+), 118 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index e459a55f21f551..43f3bc987804ce 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -1220,111 +1220,75 @@ internal static int IndexOfOrdinalIgnoreCaseNew(ReadOnlySpan source, ReadO /// The following IndexOf overload is mainly used by String.Replace. This overload assumes the parameters are already validated /// and the caller is passing a valid matchLengthPtr pointer. /// - internal unsafe int IndexOf(string source, string value, int startIndex, int count, CompareOptions options, int* matchLengthPtr, bool fromBeginning = true) + internal unsafe int IndexOf(ReadOnlySpan source, ReadOnlySpan value, int* matchLengthPtr, CompareOptions options, bool fromBeginning) { - Debug.Assert(source != null); - Debug.Assert(value != null); - Debug.Assert(startIndex >= 0); - - if (matchLengthPtr != null) - { - *matchLengthPtr = 0; - } + Debug.Assert(matchLengthPtr != null); + *matchLengthPtr = 0; - if (value.Length == 0) - { - return startIndex; - } - - if (startIndex >= source.Length) + if ((options & ValidIndexMaskOffFlags) == 0) { - return -1; - } + // Common case: caller is attempting to perform a linguistic search. + // Pass the flags down to NLS or ICU unless we're running in invariant + // mode, at which point we normalize the flags to Orginal[IgnoreCase]. - if (options == CompareOptions.OrdinalIgnoreCase) - { - int res; - if (fromBeginning) + if (!GlobalizationMode.Invariant) { - res = IndexOfOrdinal(source, value, startIndex, count, ignoreCase: true); + if (value.IsEmpty) + { + // empty target substring trivially occurs at beginning / end of search space + return (fromBeginning) ? 0 : source.Length; + } + else + { + return IndexOfCore(source, value, options, matchLengthPtr, fromBeginning); + } } - else + else if ((options & CompareOptions.IgnoreCase) == 0) { - res = LastIndexOfOrdinal(source, value, startIndex, count, ignoreCase: true); + goto ReturnOrdinal; } - - if (res >= 0 && matchLengthPtr != null) + else { - *matchLengthPtr = value.Length; + goto ReturnOrdinalIgnoreCase; } - return res; } - - if (GlobalizationMode.Invariant) + else { - bool ignoreCase = (options & (CompareOptions.IgnoreCase | CompareOptions.OrdinalIgnoreCase)) != 0; - int res; + // Less common case: caller is attempting to perform non-linguistic comparison, + // or an invalid combination of flags was supplied. - if (fromBeginning) + if (options == CompareOptions.Ordinal) { - res = IndexOfOrdinal(source, value, startIndex, count, ignoreCase); + goto ReturnOrdinal; } - else + else if (options == CompareOptions.OrdinalIgnoreCase) { - res = LastIndexOfOrdinal(source, value, startIndex, count, ignoreCase); + goto ReturnOrdinalIgnoreCase; } - - if (res >= 0 && matchLengthPtr != null) + else { - *matchLengthPtr = value.Length; + ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_InvalidFlag, ExceptionArgument.options); } - return res; } - if (options == CompareOptions.Ordinal) - { - int retValue; + ReturnOrdinal: + int retVal = (fromBeginning) ? source.IndexOf(value) : source.LastIndexOf(value); + goto OrdinalReturn; - if (fromBeginning) - { - retValue = SpanHelpers.IndexOf( - ref Unsafe.Add(ref source.GetRawStringData(), startIndex), - count, - ref value.GetRawStringData(), - value.Length); - } - else - { - retValue = SpanHelpers.LastIndexOf( - ref Unsafe.Add(ref source.GetRawStringData(), startIndex), - count, - ref value.GetRawStringData(), - value.Length); - } + ReturnOrdinalIgnoreCase: + retVal = IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning); + goto OrdinalReturn; - if (retValue >= 0) - { - retValue += startIndex; - if (matchLengthPtr != null) - { - *matchLengthPtr = value.Length; - } - } + OrdinalReturn: + // Both Ordinal and OrdinalIgnoreCase match by individual code points in a non-linguistic manner. + // Non-BMP code points will never match BMP code points, so given UTF-16 inputs the match length + // will always be equivalent to the target string length. - return retValue; - } - else + if (retVal >= 0) { - if (fromBeginning) - { - // Call the string-based overload, as it special-cases IsFastSort as a perf optimization. - return IndexOfCore(source, value, startIndex, count, options, matchLengthPtr); - } - else - { - return IndexOfCore(source.AsSpan(startIndex, count), value, options, matchLengthPtr, fromBeginning: false); - } + *matchLengthPtr = value.Length; } + return retVal; } internal static int IndexOfOrdinal(string source, string value, int startIndex, int count, bool ignoreCase) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index a15d9b99950248..927959e77c9a72 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.Globalization; using System.Numerics; +using System.Runtime.InteropServices.WindowsRuntime; using System.Text; using Internal.Runtime.CompilerServices; @@ -968,7 +969,7 @@ public string Remove(int startIndex) public string Replace(string oldValue, string? newValue, bool ignoreCase, CultureInfo? culture) { - return ReplaceCore(oldValue, newValue, culture, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None); + return ReplaceCore(oldValue, newValue, culture?.CompareInfo, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None); } public string Replace(string oldValue, string? newValue, StringComparison comparisonType) @@ -977,78 +978,92 @@ public string Replace(string oldValue, string? newValue, StringComparison compar { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return ReplaceCore(oldValue, newValue, CultureInfo.CurrentCulture, GetCaseCompareOfComparisonCulture(comparisonType)); + return ReplaceCore(oldValue, newValue, CultureInfo.CurrentCulture.CompareInfo, GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return ReplaceCore(oldValue, newValue, CultureInfo.InvariantCulture, GetCaseCompareOfComparisonCulture(comparisonType)); + return ReplaceCore(oldValue, newValue, CompareInfo.Invariant, GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.Ordinal: return Replace(oldValue, newValue); case StringComparison.OrdinalIgnoreCase: - return ReplaceCore(oldValue, newValue, CultureInfo.InvariantCulture, CompareOptions.OrdinalIgnoreCase); + return ReplaceCore(oldValue, newValue, CompareInfo.Invariant, CompareOptions.OrdinalIgnoreCase); default: throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType)); } } - private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo? culture, CompareOptions options) + private string ReplaceCore(string oldValue, string? newValue, CompareInfo? ci, CompareOptions options) { - if (oldValue == null) + if (oldValue is null) + { throw new ArgumentNullException(nameof(oldValue)); + } + if (oldValue.Length == 0) + { throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue)); + } // If they asked to replace oldValue with a null, replace all occurrences - // with the empty string. - newValue ??= string.Empty; + // with the empty string. AsSpan() will normalize appropriately. + // + // If inner ReplaceCore method returns null, it means no substitutions were + // performed, so as an optimization we'll return the original string. - CultureInfo referenceCulture = culture ?? CultureInfo.CurrentCulture; - var result = new ValueStringBuilder(stackalloc char[256]); - result.EnsureCapacity(this.Length); + return ReplaceCore(this, oldValue.AsSpan(), newValue.AsSpan(), ci ?? CultureInfo.CurrentCulture.CompareInfo, options) + ?? this; + } - int startIndex = 0; - int index = 0; + private static unsafe string? ReplaceCore(ReadOnlySpan searchSpace, ReadOnlySpan oldValue, ReadOnlySpan newValue, CompareInfo compareInfo, CompareOptions options) + { + Debug.Assert(!oldValue.IsEmpty); + Debug.Assert(compareInfo != null); - int matchLength = 0; + var result = new ValueStringBuilder(stackalloc char[256]); + result.EnsureCapacity(searchSpace.Length); + int matchLength = 0; bool hasDoneAnyReplacements = false; - CompareInfo ci = referenceCulture.CompareInfo; - do + while (true) { - index = ci.IndexOf(this, oldValue, startIndex, this.Length - startIndex, options, &matchLength); + int index = compareInfo.IndexOf(searchSpace, oldValue, &matchLength, options, fromBeginning: true); // There's the possibility that 'oldValue' has zero collation weight (empty string equivalent). // If this is the case, we behave as if there are no more substitutions to be made. - if (index >= 0 && matchLength > 0) + if (index < 0 || matchLength == 0) { - // append the unmodified portion of string - result.Append(this.AsSpan(startIndex, index - startIndex)); + break; + } - // append the replacement - result.Append(newValue); + // append the unmodified portion of search space + result.Append(searchSpace.Slice(0, index)); - startIndex = index + matchLength; - hasDoneAnyReplacements = true; - } - else if (!hasDoneAnyReplacements) - { - // small optimization, - // if we have not done any replacements, - // we will return the original string - result.Dispose(); - return this; - } - else - { - result.Append(this.AsSpan(startIndex, this.Length - startIndex)); - } - } while (index >= 0); + // append the replacement + result.Append(newValue); + + searchSpace = searchSpace.Slice(index + matchLength); + hasDoneAnyReplacements = true; + } + + // Didn't find 'oldValue' in the remaining search space, or the match + // consisted only of zero collation weight characters. As an optimization, + // if we have not yet performed any replacements, we'll save the + // allocation. + + if (!hasDoneAnyReplacements) + { + result.Dispose(); + return null; + } + + // Append what remains of the search space, then allocate the new string. + result.Append(searchSpace); return result.ToString(); } diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Searching.cs b/src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Searching.cs index ea295dfa26c4d4..cfaaaf8663fffe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Searching.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Searching.cs @@ -281,7 +281,7 @@ private unsafe bool TryFind(Utf8Span value, StringComparison comparisonType, out } else { - idx = compareInfo.IndexOf(thisTranscodedToUtf16, otherTranscodedToUtf16, 0, thisTranscodedToUtf16.Length, compareOptions, &matchLength, fromBeginning); + idx = compareInfo.IndexOf(thisTranscodedToUtf16, otherTranscodedToUtf16, &matchLength, compareOptions, fromBeginning); } #else Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); From bf8209fa7870a06e4d36d1e990f91497d1dc82a9 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 11:17:58 -0700 Subject: [PATCH 11/30] Plumb string.[Last]IndexOf through CompareInfo always --- .../src/System/String.Searching.cs | 43 +++---------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs b/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs index 93de7fa440259b..77162ee23a61a5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs @@ -310,16 +310,6 @@ public int IndexOf(string value, int startIndex) public int IndexOf(string value, int startIndex, int count) { - if (startIndex < 0 || startIndex > this.Length) - { - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - } - - if (count < 0 || count > this.Length - startIndex) - { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); - } - return IndexOf(value, startIndex, count, StringComparison.CurrentCulture); } @@ -335,26 +325,7 @@ public int IndexOf(string value, int startIndex, StringComparison comparisonType public int IndexOf(string value, int startIndex, int count, StringComparison comparisonType) { - // Validate inputs - if (value == null) - throw new ArgumentNullException(nameof(value)); - - if (startIndex < 0 || startIndex > this.Length) - throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_Index); - - if (count < 0 || startIndex > this.Length - count) - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); - - if (comparisonType == StringComparison.Ordinal) - { - int result = SpanHelpers.IndexOf( - ref Unsafe.Add(ref this._firstChar, startIndex), - count, - ref value._firstChar, - value.Length); - - return (result >= 0 ? startIndex : 0) + result; - } + // Parameter checking will be done by CompareInfo.IndexOf. switch (comparisonType) { @@ -366,11 +337,14 @@ ref Unsafe.Add(ref this._firstChar, startIndex), case StringComparison.InvariantCultureIgnoreCase: return CompareInfo.Invariant.IndexOf(this, value, startIndex, count, GetCaseCompareOfComparisonCulture(comparisonType)); + case StringComparison.Ordinal: case StringComparison.OrdinalIgnoreCase: - return CompareInfo.IndexOfOrdinal(this, value, startIndex, count, GetCaseCompareOfComparisonCulture(comparisonType) != CompareOptions.None); + return CompareInfo.Invariant.IndexOf(this, value, startIndex, count, GetCompareOptionsFromOrdinalStringComparison(comparisonType)); default: - throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType)); + throw (value is null) + ? new ArgumentNullException(nameof(value)) + : new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType)); } } @@ -498,11 +472,6 @@ public int LastIndexOf(string value, int startIndex) public int LastIndexOf(string value, int startIndex, int count) { - if (count < 0) - { - throw new ArgumentOutOfRangeException(nameof(count), SR.ArgumentOutOfRange_Count); - } - return LastIndexOf(value, startIndex, count, StringComparison.CurrentCulture); } From ed180c0884134880b7782d265391ee2eb2bd8d49 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 11:51:55 -0700 Subject: [PATCH 12/30] Remove workaround for #8890 --- .../System/Globalization/CompareInfo.Unix.cs | 19 -------- .../Globalization/CompareInfo.Windows.cs | 48 ------------------- .../src/System/Globalization/CompareInfo.cs | 42 ---------------- 3 files changed, 109 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs index f809db19702177..562a0ab586bd76 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs @@ -165,25 +165,6 @@ private static unsafe int CompareStringOrdinalIgnoreCase(ref char string1, int c } } - // TODO https://github.com/dotnet/runtime/issues/8890: - // This method shouldn't be necessary, as we should be able to just use the overload - // that takes two spans. But due to this issue, that's adding significant overhead. - private unsafe int CompareString(ReadOnlySpan string1, string string2, CompareOptions options) - { - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(string2 != null); - Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); - - // Unlike NLS, ICU (ucol_getSortKey) allows passing nullptr for either of the source arguments - // as long as the corresponding length parameter is 0. - - fixed (char* pString1 = &MemoryMarshal.GetReference(string1)) - fixed (char* pString2 = &string2.GetRawStringData()) - { - return Interop.Globalization.CompareString(_sortHandle, pString1, string1.Length, pString2, string2.Length, options); - } - } - private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index bc814e4a399721..eff4010e91aecd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -189,54 +189,6 @@ private static unsafe int CompareStringOrdinalIgnoreCase(ref char string1, int c } } - // TODO https://github.com/dotnet/runtime/issues/8890: - // This method shouldn't be necessary, as we should be able to just use the overload - // that takes two spans. But due to this issue, that's adding significant overhead. - private unsafe int CompareString(ReadOnlySpan string1, string string2, CompareOptions options) - { - Debug.Assert(string2 != null); - Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); - - string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName; - - // CompareStringEx may try to dereference the first character of its input, even if an explicit - // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs - // are normalized to a null-terminated empty string. - - if (string1.IsEmpty) - { - string1 = string.Empty; - } - - fixed (char* pLocaleName = localeName) - fixed (char* pString1 = &MemoryMarshal.GetReference(string1)) - fixed (char* pString2 = &string2.GetPinnableReference()) - { - Debug.Assert(*pString1 >= 0); // assert that we can always dereference this - Debug.Assert(*pString2 >= 0); // assert that we can always dereference this - - int result = Interop.Kernel32.CompareStringEx( - pLocaleName, - (uint)GetNativeCompareFlags(options), - pString1, - string1.Length, - pString2, - string2.Length, - null, - null, - _sortHandle); - - if (result == 0) - { - throw new ArgumentException(SR.Arg_ExternalException); - } - - // Map CompareStringEx return value to -1, 0, 1. - return result - 2; - } - } - private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 43f3bc987804ce..43ddb228d4363c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -298,48 +298,6 @@ public int Compare(string? string1, string? string2, CompareOptions options) return retVal; } - // TODO https://github.com/dotnet/runtime/issues/8890: - // This method shouldn't be necessary, as we should be able to just use the overload - // that takes two spans. But due to this issue, that's adding significant overhead. - internal int Compare(ReadOnlySpan string1, string? string2, CompareOptions options) - { - if (options == CompareOptions.OrdinalIgnoreCase) - { - return CompareOrdinalIgnoreCase(string1, string2.AsSpan()); - } - - // Verify the options before we do any real comparison. - if ((options & CompareOptions.Ordinal) != 0) - { - if (options != CompareOptions.Ordinal) - { - throw new ArgumentException(SR.Argument_CompareOptionOrdinal, nameof(options)); - } - - return string.CompareOrdinal(string1, string2.AsSpan()); - } - - if ((options & ValidCompareMaskOffFlags) != 0) - { - throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options)); - } - - // null sorts less than any other string. - if (string2 == null) - { - return 1; - } - - if (GlobalizationMode.Invariant) - { - return (options & CompareOptions.IgnoreCase) != 0 ? - CompareOrdinalIgnoreCase(string1, string2.AsSpan()) : - string.CompareOrdinal(string1, string2.AsSpan()); - } - - return CompareString(string1, string2, options); - } - internal int CompareOptionIgnoreCase(ReadOnlySpan string1, ReadOnlySpan string2) { return GlobalizationMode.Invariant ? From c25e5c1a426ae642da12afb3374bb99788480c84 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 13:08:24 -0700 Subject: [PATCH 13/30] Add ref asms --- src/libraries/System.Runtime/ref/System.Runtime.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 52b631512e1da7..cf9b862b8252e6 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -5739,6 +5739,7 @@ internal CompareInfo() { } public int LCID { get { throw null; } } public string Name { get { throw null; } } public System.Globalization.SortVersion Version { get { throw null; } } + public int Compare(System.ReadOnlySpan string1, System.ReadOnlySpan string2, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int Compare(string? string1, int offset1, int length1, string? string2, int offset2, int length2) { throw null; } public int Compare(string? string1, int offset1, int length1, string? string2, int offset2, int length2, System.Globalization.CompareOptions options) { throw null; } public int Compare(string? string1, int offset1, string? string2, int offset2) { throw null; } @@ -5755,6 +5756,8 @@ internal CompareInfo() { } public int GetHashCode(string source, System.Globalization.CompareOptions options) { throw null; } public System.Globalization.SortKey GetSortKey(string source) { throw null; } public System.Globalization.SortKey GetSortKey(string source, System.Globalization.CompareOptions options) { throw null; } + public int GetSortKey(System.ReadOnlySpan source, System.Span sortKey, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int GetSortKeyLength(System.ReadOnlySpan source, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int IndexOf(string source, char value) { throw null; } public int IndexOf(string source, char value, System.Globalization.CompareOptions options) { throw null; } public int IndexOf(string source, char value, int startIndex) { throw null; } @@ -5767,12 +5770,18 @@ internal CompareInfo() { } public int IndexOf(string source, string value, int startIndex, System.Globalization.CompareOptions options) { throw null; } public int IndexOf(string source, string value, int startIndex, int count) { throw null; } public int IndexOf(string source, string value, int startIndex, int count, System.Globalization.CompareOptions options) { throw null; } + public int IndexOfNew(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int IndexOfNew(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public bool IsPrefix(string source, string prefix) { throw null; } public bool IsPrefix(string source, string prefix, System.Globalization.CompareOptions options) { throw null; } + public bool IsPrefixNew(System.ReadOnlySpan source, System.ReadOnlySpan prefix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public static bool IsSortable(char ch) { throw null; } + public static bool IsSortable(System.ReadOnlySpan text) { throw null; } public static bool IsSortable(string text) { throw null; } + public static bool IsSortable(System.Text.Rune value) { throw null; } public bool IsSuffix(string source, string suffix) { throw null; } public bool IsSuffix(string source, string suffix, System.Globalization.CompareOptions options) { throw null; } + public bool IsSuffixNew(System.ReadOnlySpan source, System.ReadOnlySpan suffix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int LastIndexOf(string source, char value) { throw null; } public int LastIndexOf(string source, char value, System.Globalization.CompareOptions options) { throw null; } public int LastIndexOf(string source, char value, int startIndex) { throw null; } @@ -5785,6 +5794,8 @@ internal CompareInfo() { } public int LastIndexOf(string source, string value, int startIndex, System.Globalization.CompareOptions options) { throw null; } public int LastIndexOf(string source, string value, int startIndex, int count) { throw null; } public int LastIndexOf(string source, string value, int startIndex, int count, System.Globalization.CompareOptions options) { throw null; } + public int LastIndexOfNew(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int LastIndexOfNew(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } void System.Runtime.Serialization.IDeserializationCallback.OnDeserialization(object sender) { } public override string ToString() { throw null; } } From b913b034911796ba02350b88009507a9387d7734 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 13:48:31 -0700 Subject: [PATCH 14/30] Return 'matched length' on Unix, remove dead code --- .../Interop.Collation.cs | 2 +- .../pal_collation.c | 10 ++- .../pal_collation.h | 3 +- .../System/Globalization/CompareInfo.Unix.cs | 32 +--------- .../Globalization/CompareInfo.Windows.cs | 61 ------------------- 5 files changed, 13 insertions(+), 95 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Collation.cs b/src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Collation.cs index a59292e0fca1df..ece19be581c8bc 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Collation.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Collation.cs @@ -23,7 +23,7 @@ internal static partial class Globalization internal static extern unsafe int IndexOf(IntPtr sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options, int* matchLengthPtr); [DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_LastIndexOf")] - internal static extern unsafe int LastIndexOf(IntPtr sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options); + internal static extern unsafe int LastIndexOf(IntPtr sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options, int* matchLengthPtr); [DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_IndexOfOrdinalIgnoreCase")] internal static extern unsafe int IndexOfOrdinalIgnoreCase(string target, int cwTargetLength, char* pSource, int cwSourceLength, bool findLast); diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c index 081a01693ecc83..3b6bdad4e07842 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c @@ -499,7 +499,8 @@ int32_t GlobalizationNative_LastIndexOf( int32_t cwTargetLength, const UChar* lpSource, int32_t cwSourceLength, - int32_t options) + int32_t options, + int32_t* pMatchedLength) { int32_t result = USEARCH_DONE; UErrorCode err = U_ZERO_ERROR; @@ -512,6 +513,13 @@ int32_t GlobalizationNative_LastIndexOf( if (U_SUCCESS(err)) { result = usearch_last(pSearch, &err); + + // if the search was successful, + // we'll try to get the matched string length. + if (result != USEARCH_DONE && pMatchedLength != NULL) + { + *pMatchedLength = usearch_getMatchedLength(pSearch); + } usearch_close(pSearch); } } diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.h b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.h index 3d04ba735517f8..79f2fd72ff0b56 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.h +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.h @@ -37,7 +37,8 @@ PALEXPORT int32_t GlobalizationNative_LastIndexOf(SortHandle* pSortHandle, int32_t cwTargetLength, const UChar* lpSource, int32_t cwSourceLength, - int32_t options); + int32_t options, + int32_t* pMatchedLength); PALEXPORT int32_t GlobalizationNative_IndexOfOrdinalIgnoreCase(const UChar* lpTarget, int32_t cwTargetLength, diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs index 562a0ab586bd76..5e363d7ecf9cac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs @@ -180,36 +180,6 @@ private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan } } - internal unsafe int IndexOfCore(string source, string target, int startIndex, int count, CompareOptions options, int* matchLengthPtr) - { - Debug.Assert(!GlobalizationMode.Invariant); - - Debug.Assert(!string.IsNullOrEmpty(source)); - Debug.Assert(target != null); - Debug.Assert((options & CompareOptions.OrdinalIgnoreCase) == 0); - Debug.Assert((options & CompareOptions.Ordinal) == 0); - - int index; - - if (_isAsciiEqualityOrdinal && CanUseAsciiOrdinalForOptions(options)) - { - if ((options & CompareOptions.IgnoreCase) != 0) - index = IndexOfOrdinalIgnoreCaseHelper(source.AsSpan(startIndex, count), target.AsSpan(), options, matchLengthPtr, fromBeginning: true); - else - index = IndexOfOrdinalHelper(source.AsSpan(startIndex, count), target.AsSpan(), options, matchLengthPtr, fromBeginning: true); - } - else - { - fixed (char* pSource = source) - fixed (char* pTarget = target) - { - index = Interop.Globalization.IndexOf(_sortHandle, pTarget, target.Length, pSource + startIndex, count, options, matchLengthPtr); - } - } - - return index != -1 ? index + startIndex : -1; - } - // For now, this method is only called from Span APIs with either options == CompareOptions.None or CompareOptions.IgnoreCase internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan target, CompareOptions options, int* matchLengthPtr, bool fromBeginning) { @@ -232,7 +202,7 @@ internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan ta if (fromBeginning) return Interop.Globalization.IndexOf(_sortHandle, pTarget, target.Length, pSource, source.Length, options, matchLengthPtr); else - return Interop.Globalization.LastIndexOf(_sortHandle, pTarget, target.Length, pSource, source.Length, options); + return Interop.Globalization.LastIndexOf(_sortHandle, pTarget, target.Length, pSource, source.Length, options, matchLengthPtr); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index eff4010e91aecd..a4da41629272fb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -289,23 +289,6 @@ private unsafe int FindString( } } - internal unsafe int IndexOfCore(string source, string target, int startIndex, int count, CompareOptions options, int* matchLengthPtr) - { - Debug.Assert(!GlobalizationMode.Invariant); - - Debug.Assert(target != null); - Debug.Assert((options & CompareOptions.OrdinalIgnoreCase) == 0); - Debug.Assert((options & CompareOptions.Ordinal) == 0); - - int retValue = FindString(FIND_FROMSTART | (uint)GetNativeCompareFlags(options), source.AsSpan(startIndex, count), target, matchLengthPtr); - if (retValue >= 0) - { - return retValue + startIndex; - } - - return -1; - } - internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan target, CompareOptions options, int* matchLengthPtr, bool fromBeginning) { Debug.Assert(!GlobalizationMode.Invariant); @@ -348,50 +331,6 @@ private unsafe bool EndsWith(ReadOnlySpan source, ReadOnlySpan suffi private const int FIND_FROMSTART = 0x00400000; private const int FIND_FROMEND = 0x00800000; - // TODO: Instead of this method could we just have upstack code call LastIndexOfOrdinal with ignoreCase = false? - private static unsafe int FastLastIndexOfString(string source, string target, int startIndex, int sourceCount, int targetCount) - { - int retValue = -1; - - int sourceStartIndex = startIndex - sourceCount + 1; - - fixed (char* pSource = source, spTarget = target) - { - char* spSubSource = pSource + sourceStartIndex; - - int endPattern = sourceCount - targetCount; - if (endPattern < 0) - return -1; - - Debug.Assert(target.Length >= 1); - char patternChar0 = spTarget[0]; - for (int ctrSrc = endPattern; ctrSrc >= 0; ctrSrc--) - { - if (spSubSource[ctrSrc] != patternChar0) - continue; - - int ctrPat; - for (ctrPat = 1; ctrPat < targetCount; ctrPat++) - { - if (spSubSource[ctrSrc + ctrPat] != spTarget[ctrPat]) - break; - } - if (ctrPat == targetCount) - { - retValue = ctrSrc; - break; - } - } - - if (retValue >= 0) - { - retValue += startIndex - sourceCount + 1; - } - } - - return retValue; - } - private unsafe SortKey CreateSortKey(string source, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); From cfbd8d3e4db8917c7bc3f166cf5d48ad4e042251 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 13:55:16 -0700 Subject: [PATCH 15/30] Fill in CompareInfo.IsPrefix|Suffix tests --- .../tests/CompareInfo/CompareInfoTests.IsPrefix.cs | 14 ++++++++++++++ .../tests/CompareInfo/CompareInfoTests.IsSuffix.cs | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs index a9a93c5093468b..2df29cf40dae9b 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; using Xunit; @@ -56,6 +57,9 @@ public static IEnumerable IsPrefix_TestData() yield return new object[] { s_invariantCompare, "o\u0308", "o", CompareOptions.Ordinal, true }; yield return new object[] { s_invariantCompare, "o\u0000\u0308", "o", CompareOptions.None, true }; + // Weightless comparisons + yield return new object[] { s_invariantCompare, "", "\u200d", CompareOptions.None, true }; + // Surrogates yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uD800\uDC00", CompareOptions.None, true }; yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uD800\uDC00", CompareOptions.IgnoreCase, true }; @@ -101,6 +105,16 @@ public void IsPrefix(CompareInfo compareInfo, string source, string value, Compa Assert.Equal(expected, source.StartsWith(value, stringComparison)); Assert.Equal(expected, source.AsSpan().StartsWith(value.AsSpan(), stringComparison)); } + + // Now test the span version - use BoundedMemory to detect buffer overruns + + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + + using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); + valueBoundedMemory.MakeReadonly(); + + Assert.Equal(expected, compareInfo.IsPrefixNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); } [Fact] diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs index 3d920dd917855f..8f2376ee475e16 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; using Xunit; @@ -63,6 +64,9 @@ public static IEnumerable IsSuffix_TestData() yield return new object[] { s_invariantCompare, "o\u0308o", "o", CompareOptions.None, true }; yield return new object[] { s_invariantCompare, "o\u0308o", "o", CompareOptions.Ordinal, true }; + // Weightless comparisons + yield return new object[] { s_invariantCompare, "", "\u200d", CompareOptions.None, true }; + // Surrogates yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uD800\uDC00", CompareOptions.None, true }; yield return new object[] { s_invariantCompare, "\uD800\uDC00", "\uD800\uDC00", CompareOptions.IgnoreCase, true }; @@ -104,6 +108,16 @@ public void IsSuffix(CompareInfo compareInfo, string source, string value, Compa Assert.Equal(expected, source.EndsWith(value, stringComparison)); Assert.Equal(expected, source.AsSpan().EndsWith(value.AsSpan(), stringComparison)); } + + // Now test the span version - use BoundedMemory to detect buffer overruns + + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + + using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); + valueBoundedMemory.MakeReadonly(); + + Assert.Equal(expected, compareInfo.IsSuffixNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); } [Fact] From 25868a6ca67448395e4b3d8aa1fd0f7fbcaa30dd Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 16:41:00 -0700 Subject: [PATCH 16/30] More CompareInfo test additions --- .../Common/tests/Tests/System/StringTests.cs | 13 +++ .../CompareInfo/CompareInfoTests.Compare.cs | 17 +++ .../CompareInfo/CompareInfoTests.IndexOf.cs | 39 +++++-- .../CompareInfoTests.LastIndexOf.cs | 60 ++++++++-- .../tests/CompareInfo/CompareInfoTests.cs | 36 ++++++ .../tests/Invariant/Invariant.Tests.csproj | 1 + .../tests/Invariant/InvariantMode.cs | 106 +++++++++++++++++- 7 files changed, 253 insertions(+), 19 deletions(-) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 18f494d9440e4d..36de8718c61a83 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -6765,6 +6765,19 @@ public static void StartEndWithTest(string source, string start, string end, str Assert.Equal(expected, source.EndsWith(end, ignoreCase, ci)); } + [Theory] + [InlineData("", StringComparison.InvariantCulture, true)] + [InlineData("", StringComparison.Ordinal, true)] + [InlineData(ZeroWidthJoiner, StringComparison.InvariantCulture, true)] + [InlineData(ZeroWidthJoiner, StringComparison.Ordinal, false)] + public static void StartEndWith_ZeroWeightValue(string value, StringComparison comparison, bool expectedStartsAndEndsWithResult) + { + Assert.Equal(expectedStartsAndEndsWithResult, string.Empty.StartsWith(value, comparison)); + Assert.Equal(expectedStartsAndEndsWithResult, string.Empty.EndsWith(value, comparison)); + Assert.Equal(expectedStartsAndEndsWithResult ? 0 : -1, string.Empty.IndexOf(value, comparison)); + Assert.Equal(expectedStartsAndEndsWithResult ? 0 : -1, string.Empty.LastIndexOf(value, comparison)); + } + [Fact] public static void StartEndNegativeTest() { diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs index 3b2f121f586b7a..1b1647a2e379c0 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; using Xunit; @@ -361,6 +362,22 @@ public void Compare_Advanced(CompareInfo compareInfo, string string1, int offset // Use Compare(string, int, int, string, int, int, CompareOptions) Assert.Equal(expected, Math.Sign(compareInfo.Compare(string1, offset1, length1, string2, offset2, length2, options))); Assert.Equal(-expected, Math.Sign(compareInfo.Compare(string2, offset2, length2, string1, offset1, length1, options))); + + // Now test the span-based versions - use BoundedMemory to detect buffer overruns + + RunSpanCompareTest(compareInfo, string1.AsSpan(offset1, length1), string2.AsSpan(offset2, length2), options, expected); + + static void RunSpanCompareTest(CompareInfo compareInfo, ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options, int expected) + { + using BoundedMemory string1BoundedMemory = BoundedMemory.AllocateFromExistingData(string1); + string1BoundedMemory.MakeReadonly(); + + using BoundedMemory string2BoundedMemory = BoundedMemory.AllocateFromExistingData(string2); + string2BoundedMemory.MakeReadonly(); + + Assert.Equal(expected, Math.Sign(compareInfo.Compare(string1, string2, options))); + Assert.Equal(-expected, Math.Sign(compareInfo.Compare(string2, string1, options))); + } } [Fact] diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs index 2036dc4d80ce17..e4fd1753b81ed2 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs @@ -2,7 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; +using System.Text; using Xunit; namespace System.Globalization.Tests @@ -63,6 +65,10 @@ public static IEnumerable IndexOf_TestData() yield return new object[] { s_invariantCompare, "TestFooBA\u0300R", "FooB\u00C0R", 0, 11, CompareOptions.IgnoreNonSpace, 4 }; yield return new object[] { s_invariantCompare, "o\u0308", "o", 0, 2, CompareOptions.None, -1 }; + // Weightless characters + yield return new object[] { s_invariantCompare, "", "\u200d", 0, 0, CompareOptions.None, 0 }; + yield return new object[] { s_invariantCompare, "hello", "\u200d", 1, 3, CompareOptions.IgnoreCase, 1 }; + // Ignore symbols yield return new object[] { s_invariantCompare, "More Test's", "Tests", 0, 11, CompareOptions.IgnoreSymbols, 5 }; yield return new object[] { s_invariantCompare, "More Test's", "Tests", 0, 11, CompareOptions.None, -1 }; @@ -192,7 +198,27 @@ public void IndexOf_String(CompareInfo compareInfo, string source, string value, // Use int MemoryExtensions.IndexOf(this ReadOnlySpan, ReadOnlySpan, StringComparison) Assert.Equal((expected == -1) ? -1 : (expected - startIndex), source.AsSpan(startIndex, count).IndexOf(value.AsSpan(), stringComparison)); } - } + + // Now test the span-based versions - use BoundedMemory to detect buffer overruns + + RunSpanIndexOfTest(compareInfo, source.AsSpan(startIndex, count), value, options, (expected < 0) ? expected : expected - startIndex); + + static void RunSpanIndexOfTest(CompareInfo compareInfo, ReadOnlySpan source, ReadOnlySpan value, CompareOptions options, int expected) + { + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + + using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); + valueBoundedMemory.MakeReadonly(); + + Assert.Equal(expected, compareInfo.IndexOfNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + + if (TryCreateRuneFrom(value, out Rune rune)) + { + Assert.Equal(expected, compareInfo.IndexOfNew(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version + } + } + } private static void IndexOf_Char(CompareInfo compareInfo, string source, char value, int startIndex, int count, CompareOptions options, int expected) { @@ -331,14 +357,11 @@ public void IndexOf_Invalid() AssertExtensions.Throws("count", () => s_invariantCompare.IndexOf("Test", 'a', 2, 4, CompareOptions.None)); } - [Fact] - public static void IndexOf_MinusOneCompatability() + // Attempts to create a Rune from the entirety of a given text buffer. + private static bool TryCreateRuneFrom(ReadOnlySpan text, out Rune value) { - // This behavior was for .NET Framework 1.1 compatability. - // Allowing empty source strings with invalid offsets was quickly outed. - // with invalid offsets. - Assert.Equal(0, s_invariantCompare.IndexOf("", "", -1, CompareOptions.None)); - Assert.Equal(-1, s_invariantCompare.IndexOf("", "a", -1, CompareOptions.None)); + return Rune.DecodeFromUtf16(text, out value, out int charsConsumed) == OperationStatus.Done + && charsConsumed == text.Length; } } } diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs index 4d0474e7d4a7b6..791058c271cd19 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs @@ -2,7 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; +using System.Text; using Xunit; namespace System.Globalization.Tests @@ -75,6 +77,11 @@ public static IEnumerable LastIndexOf_TestData() yield return new object[] { s_invariantCompare, "TestFooBA\u0300R", "FooB\u00C0R", 10, 11, CompareOptions.IgnoreNonSpace, 4 }; yield return new object[] { s_invariantCompare, "o\u0308", "o", 1, 2, CompareOptions.None, -1 }; + // Weightless characters + yield return new object[] { s_invariantCompare, "", "\u200d", 0, 0, CompareOptions.None, 0 }; + yield return new object[] { s_invariantCompare, "", "\u200d", -1, 0, CompareOptions.None, 0 }; + yield return new object[] { s_invariantCompare, "hello", "\u200d", 4, 5, CompareOptions.IgnoreCase, 5 }; + // Ignore symbols yield return new object[] { s_invariantCompare, "More Test's", "Tests", 10, 11, CompareOptions.IgnoreSymbols, 5 }; yield return new object[] { s_invariantCompare, "More Test's", "Tests", 10, 11, CompareOptions.None, -1 }; @@ -193,6 +200,26 @@ public void LastIndexOf_String(CompareInfo compareInfo, string source, string va // Use int MemoryExtensions.LastIndexOf(this ReadOnlySpan, ReadOnlySpan, StringComparison) Assert.Equal(expected - adjustmentFactor, sourceSpan.LastIndexOf(value.AsSpan(), stringComparison)); } + + // Now test the span-based versions - use BoundedMemory to detect buffer overruns + + RunSpanLastIndexOfTest(compareInfo, sourceSpan, value, options, expected - adjustmentFactor); + + static void RunSpanLastIndexOfTest(CompareInfo compareInfo, ReadOnlySpan source, ReadOnlySpan value, CompareOptions options, int expected) + { + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + + using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); + valueBoundedMemory.MakeReadonly(); + + Assert.Equal(expected, compareInfo.LastIndexOfNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + + if (TryCreateRuneFrom(value, out Rune rune)) + { + Assert.Equal(expected, compareInfo.LastIndexOfNew(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version + } + } } private static void LastIndexOf_Char(CompareInfo compareInfo, string source, char value, int startIndex, int count, CompareOptions options, int expected) @@ -271,38 +298,38 @@ public void LastIndexOf_Invalid() // Options are invalid AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", CompareOptions.StringSort)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, CompareOptions.StringSort)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 2, CompareOptions.StringSort)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 1, CompareOptions.StringSort)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', CompareOptions.StringSort)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, CompareOptions.StringSort)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 2, CompareOptions.StringSort)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 1, CompareOptions.StringSort)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 2, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 1, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 2, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 1, CompareOptions.Ordinal | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 2, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 1, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 2, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 1, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", (CompareOptions)(-1))); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, (CompareOptions)(-1))); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 2, (CompareOptions)(-1))); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 1, (CompareOptions)(-1))); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", (CompareOptions)(-1))); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, (CompareOptions)(-1))); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 2, (CompareOptions)(-1))); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 1, (CompareOptions)(-1))); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", (CompareOptions)0x11111111)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, (CompareOptions)0x11111111)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 2, (CompareOptions)0x11111111)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", "Tests", 0, 1, (CompareOptions)0x11111111)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', (CompareOptions)0x11111111)); AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, (CompareOptions)0x11111111)); - AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 2, (CompareOptions)0x11111111)); + AssertExtensions.Throws("options", () => s_invariantCompare.LastIndexOf("Test's", 'a', 0, 1, (CompareOptions)0x11111111)); // StartIndex < 0 AssertExtensions.Throws("startIndex", () => s_invariantCompare.LastIndexOf("Test", "Test", -1, CompareOptions.None)); @@ -350,6 +377,19 @@ public void LastIndexOf_Invalid() AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", "s", 4, 7, CompareOptions.None)); AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", 's', 4, 6)); AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", 's', 4, 7, CompareOptions.None)); + + // Count > StartIndex + 1 + AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", "e", 1, 3)); + AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", "e", 1, 3, CompareOptions.None)); + AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", 'e', 1, 3)); + AssertExtensions.Throws("count", () => s_invariantCompare.LastIndexOf("Test", 'e', 1, 3, CompareOptions.None)); + } + + // Attempts to create a Rune from the entirety of a given text buffer. + private static bool TryCreateRuneFrom(ReadOnlySpan text, out Rune value) + { + return Rune.DecodeFromUtf16(text, out value, out int charsConsumed) == OperationStatus.Done + && charsConsumed == text.Length; } } } diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs index b83073a76f98a2..0bb84bb168375e 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs @@ -2,8 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; using System.Collections.Generic; using System.Reflection; +using System.Text; using Xunit; namespace System.Globalization.Tests @@ -104,6 +106,7 @@ public static IEnumerable CompareInfo_TestData() // sort before the corresponding characters that are in the block U+FF00-U+FFEF private static int s_expectedHalfToFullFormsComparison = PlatformDetection.IsWindows ? -1 : 1; + private static CompareInfo s_hungarianCompare = new CultureInfo("hu-HU").CompareInfo; private static CompareInfo s_invariantCompare = CultureInfo.InvariantCulture.CompareInfo; private static CompareInfo s_turkishCompare = new CultureInfo("tr-TR").CompareInfo; @@ -373,6 +376,33 @@ public void SortKeyTest(CompareInfo compareInfo, string string1, string string2, Assert.Equal(string1, sk1.OriginalString); Assert.Equal(string2, sk2.OriginalString); + + // Now try the span-based versions - use BoundedMemory to detect buffer overruns + + RunSpanSortKeyTest(compareInfo, string1, options, sk1.KeyData); + RunSpanSortKeyTest(compareInfo, string2, options, sk2.KeyData); + + unsafe static void RunSpanSortKeyTest(CompareInfo compareInfo, ReadOnlySpan source, CompareOptions options, byte[] expectedSortKey) + { + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + + Assert.Equal(expectedSortKey.Length, compareInfo.GetSortKeyLength(sourceBoundedMemory.Span, options)); + + using BoundedMemory sortKeyBoundedMemory = BoundedMemory.Allocate(expectedSortKey.Length); + + // First try with a destination which is too small - should result in an error + + Assert.Throws(() => compareInfo.GetSortKey(sourceBoundedMemory.Span, sortKeyBoundedMemory.Span.Slice(1), options)); + + // Next, try with a destination which is perfectly sized - should succeed + + Span sortKeyBoundedSpan = sortKeyBoundedMemory.Span; + sortKeyBoundedSpan.Clear(); + + Assert.Equal(expectedSortKey.Length, compareInfo.GetSortKey(sourceBoundedMemory.Span, sortKeyBoundedSpan, options)); + Assert.Equal(expectedSortKey, sortKeyBoundedSpan[0..expectedSortKey.Length].ToArray()); + } } [Fact] @@ -436,6 +466,12 @@ public void IsSortableTest(object sourceObj, bool expected) string source = sourceObj as string ?? new string((char[])sourceObj); Assert.Equal(expected, CompareInfo.IsSortable(source)); + // Now test the span version - use BoundedMemory to detect buffer overruns + + using BoundedMemory sourceBoundedMemory = BoundedMemory.AllocateFromExistingData(source); + sourceBoundedMemory.MakeReadonly(); + Assert.Equal(expected, CompareInfo.IsSortable(sourceBoundedMemory.Span)); + // If the string as a whole is sortable, then all chars which aren't standalone // surrogate halves must also be sortable. diff --git a/src/libraries/System.Globalization/tests/Invariant/Invariant.Tests.csproj b/src/libraries/System.Globalization/tests/Invariant/Invariant.Tests.csproj index 78f6d5db78181a..1de762de1564b5 100644 --- a/src/libraries/System.Globalization/tests/Invariant/Invariant.Tests.csproj +++ b/src/libraries/System.Globalization/tests/Invariant/Invariant.Tests.csproj @@ -2,6 +2,7 @@ $(NetCoreAppCurrent) true + true diff --git a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs index d6c7676eb1f805..937a1a1405eac0 100644 --- a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs +++ b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs @@ -3,8 +3,10 @@ // See the LICENSE file in the project root for more information. using System.Buffers; +using System.Buffers.Binary; using System.Collections.Generic; -using System.Collections; +using System.IO; +using System.Runtime.InteropServices; using System.Text; using Xunit; @@ -63,6 +65,10 @@ public static IEnumerable IndexOf_TestData() yield return new object[] { "FooBar", "Foo\u0400Bar", 0, 6, CompareOptions.Ordinal, -1 }; yield return new object[] { "TestFooBA\u0300R", "FooB\u00C0R", 0, 11, CompareOptions.IgnoreNonSpace, -1 }; + // Weightless characters + yield return new object[] { "", "\u200d", 0, 0, CompareOptions.None, -1 }; + yield return new object[] { "hello", "\u200d", 0, 5, CompareOptions.IgnoreCase, -1 }; + // Ignore symbols yield return new object[] { "More Test's", "Tests", 0, 11, CompareOptions.IgnoreSymbols, -1 }; yield return new object[] { "More Test's", "Tests", 0, 11, CompareOptions.None, -1 }; @@ -167,6 +173,11 @@ public static IEnumerable LastIndexOf_TestData() yield return new object[] { "FooBar", "Foo\u0400Bar", 5, 6, CompareOptions.Ordinal, -1 }; yield return new object[] { "TestFooBA\u0300R", "FooB\u00C0R", 10, 11, CompareOptions.IgnoreNonSpace, -1 }; + // Weightless characters + yield return new object[] { "", "\u200d", 0, 0, CompareOptions.None, -1 }; + yield return new object[] { "", "\u200d", -1, 0, CompareOptions.None, -1 }; + yield return new object[] { "hello", "\u200d", 4, 5, CompareOptions.IgnoreCase, -1 }; + // Ignore symbols yield return new object[] { "More Test's", "Tests", 10, 11, CompareOptions.IgnoreSymbols, -1 }; yield return new object[] { "More Test's", "Tests", 10, 11, CompareOptions.None, -1 }; @@ -249,6 +260,10 @@ public static IEnumerable IsSuffix_TestData() yield return new object[] { "FooBar", "Foo\u0400Bar", CompareOptions.Ordinal, false }; yield return new object[] { "FooBA\u0300R", "FooB\u00C0R", CompareOptions.IgnoreNonSpace, false }; + // Weightless characters + yield return new object[] { "", "\u200d", CompareOptions.None, false }; + yield return new object[] { "", "\u200d", CompareOptions.IgnoreCase, false }; + // Ignore symbols yield return new object[] { "More Test's", "Tests", CompareOptions.IgnoreSymbols, false }; yield return new object[] { "More Test's", "Tests", CompareOptions.None, false }; @@ -634,6 +649,77 @@ public void TestSortVersion(string cultureName) Assert.Equal(version, new CultureInfo(cultureName).CompareInfo.Version); } + [Theory] + [InlineData(0, 0)] + [InlineData(1, 2)] + [InlineData(100_000, 200_000)] + [InlineData(0x3FFF_FFFF, 0x7FFF_FFFE)] + public void TestGetSortKeyLength_Valid(int inputLength, int expectedSortKeyLength) + { + using BoundedMemory boundedMemory = BoundedMemory.Allocate(0); // AV if dereferenced + boundedMemory.MakeReadonly(); + ReadOnlySpan dummySpan = MemoryMarshal.CreateReadOnlySpan(ref MemoryMarshal.GetReference(boundedMemory.Span), inputLength); + Assert.Equal(expectedSortKeyLength, CultureInfo.InvariantCulture.CompareInfo.GetSortKeyLength(dummySpan)); + } + + [Theory] + [InlineData(0x4000_0000)] + [InlineData(int.MaxValue)] + public unsafe void TestGetSortKeyLength_OverlongArgument(int inputLength) + { + using BoundedMemory boundedMemory = BoundedMemory.Allocate(0); // AV if dereferenced + boundedMemory.MakeReadonly(); + + Assert.Throws("source", () => + { + ReadOnlySpan dummySpan = MemoryMarshal.CreateReadOnlySpan(ref MemoryMarshal.GetReference(boundedMemory.Span), inputLength); + CultureInfo.InvariantCulture.CompareInfo.GetSortKeyLength(dummySpan); + }); + } + + [Theory] + [InlineData("Hello", CompareOptions.None, "Hello")] + [InlineData("Hello", CompareOptions.IgnoreWidth, "Hello")] + [InlineData("Hello", CompareOptions.IgnoreCase, "HELLO")] + [InlineData("Hello", CompareOptions.IgnoreCase | CompareOptions.IgnoreWidth, "HELLO")] + [InlineData("Hell\u00F6", CompareOptions.None, "Hell\u00F6")] // U+00F6 = LATIN SMALL LETTER O WITH DIAERESIS + [InlineData("Hell\u00F6", CompareOptions.IgnoreCase, "HELL\u00F6")] // note the final "o with diaeresis" isn't capitalized + public unsafe void TestSortKey_FromSpan(string input, CompareOptions options, string expected) + { + byte[] expectedOutputBytes = GetExpectedInvariantOrdinalSortKey(expected); + + CompareInfo compareInfo = CultureInfo.InvariantCulture.CompareInfo; + + // First, validate that too short a buffer throws + + Assert.Throws("sortKey", () => compareInfo.GetSortKey(input, new byte[expectedOutputBytes.Length - 1], options)); + + // Next, validate that using a properly-sized buffer succeeds + // We'll use BoundedMemory to check for buffer overruns + + using BoundedMemory boundedInputMemory = BoundedMemory.AllocateFromExistingData(input); + boundedInputMemory.MakeReadonly(); + ReadOnlySpan boundedInputSpan = boundedInputMemory.Span; + + using BoundedMemory boundedOutputMemory = BoundedMemory.Allocate(expectedOutputBytes.Length); + Span boundedOutputSpan = boundedOutputMemory.Span; + + Assert.Equal(expectedOutputBytes.Length, compareInfo.GetSortKey(boundedInputSpan, boundedOutputSpan, options)); + Assert.Equal(expectedOutputBytes, boundedOutputSpan[0..expectedOutputBytes.Length].ToArray()); + + // Now try it once more, passing a larger span where the last byte points to unallocated memory. + // If GetSortKey attempts to write beyond the number of bytes we expect, the unit test will AV. + + boundedOutputSpan.Clear(); + + fixed (byte* pBoundedOutputSpan = boundedOutputSpan) + { + boundedOutputSpan = new Span(pBoundedOutputSpan, boundedOutputSpan.Length + 1); // last byte is unallocated memory + Assert.Equal(expectedOutputBytes.Length, compareInfo.GetSortKey(boundedInputSpan, boundedOutputSpan, options)); + Assert.Equal(expectedOutputBytes, boundedOutputSpan[0..expectedOutputBytes.Length].ToArray()); + } + } + [Fact] public void TestSortKey_ZeroWeightCodePoints() { @@ -767,6 +853,7 @@ public void TestIsSuffix(string source, string value, CompareOptions options, bo valueBoundedMemory.MakeReadonly(); ReadOnlySpan valueBoundedSpan = valueBoundedMemory.Span; + Assert.Equal(result, CultureInfo.GetCultureInfo(cul).CompareInfo.IsSuffixNew(sourceBoundedSpan, valueBoundedSpan, options)); Assert.Equal(result, sourceBoundedSpan.EndsWith(valueBoundedSpan, GetStringComparison(options))); } } @@ -810,6 +897,9 @@ public void TestCompare(string source, string value, CompareOptions options, int valueBoundedMemory.MakeReadonly(); ReadOnlySpan valueBoundedSpan = valueBoundedMemory.Span; + res = CultureInfo.GetCultureInfo(cul).CompareInfo.Compare(sourceBoundedSpan, valueBoundedSpan, options); + Assert.Equal(result, Math.Sign(res)); + res = sourceBoundedSpan.CompareTo(valueBoundedSpan, GetStringComparison(options)); Assert.Equal(result, Math.Sign(res)); } @@ -917,5 +1007,19 @@ public void TestRune(int original, int expectedToUpper, int expectedToLower) Assert.Equal(expectedToLower, Rune.ToLowerInvariant(originalRune).Value); Assert.Equal(expectedToLower, Rune.ToLower(originalRune, CultureInfo.GetCultureInfo("tr-TR")).Value); } + + private static byte[] GetExpectedInvariantOrdinalSortKey(ReadOnlySpan input) + { + MemoryStream memoryStream = new MemoryStream(); + Span tempBuffer = stackalloc byte[sizeof(char)]; + + foreach (char ch in input) + { + BinaryPrimitives.WriteUInt16BigEndian(tempBuffer, (ushort)ch); + memoryStream.Write(tempBuffer); + } + + return memoryStream.ToArray(); + } } } From b30e6cfe1e4ff396568b9d7e044a2b4b3051ca5b Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Apr 2020 17:35:47 -0700 Subject: [PATCH 17/30] Work around ICU usearch_* handling empty inputs incorrectly --- .../pal_collation.c | 42 +++++++++++++++++++ .../System/Globalization/CompareInfo.Unix.cs | 9 ++-- .../Globalization/CompareInfo.Windows.cs | 2 - 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c index 3b6bdad4e07842..9197ab338d89b0 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c @@ -464,7 +464,28 @@ int32_t GlobalizationNative_IndexOf( int32_t options, int32_t* pMatchedLength) { + assert(cwTargetLength > 0); + int32_t result = USEARCH_DONE; + + // It's possible somebody passed us (source = , target = ). + // ICU's usearch_* APIs don't handle empty source inputs properly. However, + // if this occurs the user really just wanted us to perform an equality check. + // We can't short-circuit the operation because depending on the collation in + // use, certain code points may have zero weight, which means that empty + // strings may compare as equal to non-empty strings. + + if (cwSourceLength == 0) + { + result = GlobalizationNative_CompareString(pSortHandle, lpTarget, cwTargetLength, lpSource, cwSourceLength, options); + if (result == UCOL_EQUAL && pMatchedLength != NULL) + { + *pMatchedLength = cwTargetLength; + } + + return (result == UCOL_EQUAL) ? 0 : -1; + } + UErrorCode err = U_ZERO_ERROR; const UCollator* pColl = GetCollatorFromSortHandle(pSortHandle, options, &err); @@ -502,7 +523,28 @@ int32_t GlobalizationNative_LastIndexOf( int32_t options, int32_t* pMatchedLength) { + assert(cwTargetLength > 0); + int32_t result = USEARCH_DONE; + + // It's possible somebody passed us (source = , target = ). + // ICU's usearch_* APIs don't handle empty source inputs properly. However, + // if this occurs the user really just wanted us to perform an equality check. + // We can't short-circuit the operation because depending on the collation in + // use, certain code points may have zero weight, which means that empty + // strings may compare as equal to non-empty strings. + + if (cwSourceLength == 0) + { + result = GlobalizationNative_CompareString(pSortHandle, lpTarget, cwTargetLength, lpSource, cwSourceLength, options); + if (result == UCOL_EQUAL && pMatchedLength != NULL) + { + *pMatchedLength = cwTargetLength; + } + + return (result == UCOL_EQUAL) ? 0 : -1; + } + UErrorCode err = U_ZERO_ERROR; const UCollator* pColl = GetCollatorFromSortHandle(pSortHandle, options, &err); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs index 5e363d7ecf9cac..54c2c7364dc266 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs @@ -170,8 +170,8 @@ private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan Debug.Assert(!GlobalizationMode.Invariant); Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); - // Unlike NLS, ICU (ucol_getSortKey) allows passing nullptr for either of the source arguments - // as long as the corresponding length parameter is 0. + // GetReference may return nullptr if the input span is defaulted. The native layer handles + // this appropriately; no workaround is needed on the managed side. fixed (char* pString1 = &MemoryMarshal.GetReference(string1)) fixed (char* pString2 = &MemoryMarshal.GetReference(string2)) @@ -180,11 +180,9 @@ private unsafe int CompareString(ReadOnlySpan string1, ReadOnlySpan } } - // For now, this method is only called from Span APIs with either options == CompareOptions.None or CompareOptions.IgnoreCase internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan target, CompareOptions options, int* matchLengthPtr, bool fromBeginning) { Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(source.Length != 0); Debug.Assert(target.Length != 0); if (_isAsciiEqualityOrdinal && CanUseAsciiOrdinalForOptions(options)) @@ -196,6 +194,9 @@ internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan ta } else { + // GetReference may return nullptr if the input span is defaulted. The native layer handles + // this appropriately; no workaround is needed on the managed side. + fixed (char* pSource = &MemoryMarshal.GetReference(source)) fixed (char* pTarget = &MemoryMarshal.GetReference(target)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs index a4da41629272fb..d0bb7775e0f7cf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs @@ -292,9 +292,7 @@ private unsafe int FindString( internal unsafe int IndexOfCore(ReadOnlySpan source, ReadOnlySpan target, CompareOptions options, int* matchLengthPtr, bool fromBeginning) { Debug.Assert(!GlobalizationMode.Invariant); - Debug.Assert(target.Length != 0); - Debug.Assert(options == CompareOptions.None || options == CompareOptions.IgnoreCase); uint positionFlag = fromBeginning ? (uint)FIND_FROMSTART : FIND_FROMEND; return FindString(positionFlag | (uint)GetNativeCompareFlags(options), source, target, matchLengthPtr); From b757f057cb40b03905504dfe03d1e28214c86520 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Sat, 11 Apr 2020 12:39:04 -0700 Subject: [PATCH 18/30] Drop 'new' prefix on newly introduced methods --- .../CompareInfo/CompareInfoTests.IndexOf.cs | 4 +- .../CompareInfo/CompareInfoTests.IsPrefix.cs | 2 +- .../CompareInfo/CompareInfoTests.IsSuffix.cs | 2 +- .../CompareInfoTests.LastIndexOf.cs | 4 +- .../tests/Invariant/InvariantMode.cs | 2 +- .../src/System/Globalization/CompareInfo.cs | 44 +++++++++---------- .../System/MemoryExtensions.Globalization.cs | 20 ++++----- .../System.Runtime/ref/System.Runtime.cs | 12 ++--- 8 files changed, 45 insertions(+), 45 deletions(-) diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs index e4fd1753b81ed2..043a4d3ad7f0a1 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IndexOf.cs @@ -211,11 +211,11 @@ static void RunSpanIndexOfTest(CompareInfo compareInfo, ReadOnlySpan sourc using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); valueBoundedMemory.MakeReadonly(); - Assert.Equal(expected, compareInfo.IndexOfNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + Assert.Equal(expected, compareInfo.IndexOf(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); if (TryCreateRuneFrom(value, out Rune rune)) { - Assert.Equal(expected, compareInfo.IndexOfNew(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version + Assert.Equal(expected, compareInfo.IndexOf(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version } } } diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs index 2df29cf40dae9b..b4acf5cbaf190c 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsPrefix.cs @@ -114,7 +114,7 @@ public void IsPrefix(CompareInfo compareInfo, string source, string value, Compa using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); valueBoundedMemory.MakeReadonly(); - Assert.Equal(expected, compareInfo.IsPrefixNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + Assert.Equal(expected, compareInfo.IsPrefix(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); } [Fact] diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs index 8f2376ee475e16..a716a7c1eb5603 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs @@ -117,7 +117,7 @@ public void IsSuffix(CompareInfo compareInfo, string source, string value, Compa using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); valueBoundedMemory.MakeReadonly(); - Assert.Equal(expected, compareInfo.IsSuffixNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + Assert.Equal(expected, compareInfo.IsSuffix(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); } [Fact] diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs index 791058c271cd19..4831b2df12acd7 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs @@ -213,11 +213,11 @@ static void RunSpanLastIndexOfTest(CompareInfo compareInfo, ReadOnlySpan s using BoundedMemory valueBoundedMemory = BoundedMemory.AllocateFromExistingData(value); valueBoundedMemory.MakeReadonly(); - Assert.Equal(expected, compareInfo.LastIndexOfNew(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); + Assert.Equal(expected, compareInfo.LastIndexOf(sourceBoundedMemory.Span, valueBoundedMemory.Span, options)); if (TryCreateRuneFrom(value, out Rune rune)) { - Assert.Equal(expected, compareInfo.LastIndexOfNew(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version + Assert.Equal(expected, compareInfo.LastIndexOf(sourceBoundedMemory.Span, rune, options)); // try the Rune-based version } } } diff --git a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs index 937a1a1405eac0..2de55dfb947353 100644 --- a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs +++ b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs @@ -853,7 +853,7 @@ public void TestIsSuffix(string source, string value, CompareOptions options, bo valueBoundedMemory.MakeReadonly(); ReadOnlySpan valueBoundedSpan = valueBoundedMemory.Span; - Assert.Equal(result, CultureInfo.GetCultureInfo(cul).CompareInfo.IsSuffixNew(sourceBoundedSpan, valueBoundedSpan, options)); + Assert.Equal(result, CultureInfo.GetCultureInfo(cul).CompareInfo.IsSuffix(sourceBoundedSpan, valueBoundedSpan, options)); Assert.Equal(result, sourceBoundedSpan.EndsWith(valueBoundedSpan, GetStringComparison(options))); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 43ddb228d4363c..ff8b358fbadb17 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -743,7 +743,7 @@ public bool IsPrefix(string source, string prefix, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.prefix); } - return IsPrefixNew(source, prefix, options); + return IsPrefix(source.AsSpan(), prefix.AsSpan(), options); } /// @@ -759,7 +759,7 @@ public bool IsPrefix(string source, string prefix, CompareOptions options) /// /// contains an unsupported combination of flags. /// - public bool IsPrefixNew(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options = CompareOptions.None) + public bool IsPrefix(ReadOnlySpan source, ReadOnlySpan prefix, CompareOptions options = CompareOptions.None) { // The empty string is trivially a prefix of every other string. For compat with // earlier versions of the Framework we'll early-exit here before validating the @@ -835,7 +835,7 @@ public bool IsSuffix(string source, string suffix, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.suffix); } - return IsSuffixNew(source, suffix, options); + return IsSuffix(source.AsSpan(), suffix.AsSpan(), options); } /// @@ -851,7 +851,7 @@ public bool IsSuffix(string source, string suffix, CompareOptions options) /// /// contains an unsupported combination of flags. /// - public bool IsSuffixNew(ReadOnlySpan source, ReadOnlySpan suffix, CompareOptions options = CompareOptions.None) + public bool IsSuffix(ReadOnlySpan source, ReadOnlySpan suffix, CompareOptions options = CompareOptions.None) { // The empty string is trivially a suffix of every other string. For compat with // earlier versions of the Framework we'll early-exit here before validating the @@ -937,7 +937,7 @@ public int IndexOf(string source, char value, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - return IndexOfNew(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + return IndexOf(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); } public int IndexOf(string source, string value, CompareOptions options) @@ -951,7 +951,7 @@ public int IndexOf(string source, string value, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - return IndexOfNew(source, value, options); + return IndexOf(source.AsSpan(), value.AsSpan(), options); } public int IndexOf(string source, char value, int startIndex) @@ -1017,7 +1017,7 @@ public unsafe int IndexOf(string source, char value, int startIndex, int count, } } - int result = IndexOfNew(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + int result = IndexOf(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); if (result >= 0) { result += startIndex; @@ -1051,7 +1051,7 @@ public unsafe int IndexOf(string source, string value, int startIndex, int count } } - int result = IndexOfNew(sourceSpan, value, options); + int result = IndexOf(sourceSpan, value, options); if (result >= 0) { result += startIndex; @@ -1072,7 +1072,7 @@ public unsafe int IndexOf(string source, string value, int startIndex, int count /// /// contains an unsupported combination of flags. /// - public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) + public unsafe int IndexOf(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) { if ((options & ValidIndexMaskOffFlags) == 0) { @@ -1123,7 +1123,7 @@ public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value return source.IndexOf(value); ReturnOrdinalIgnoreCase: - return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: true); + return IndexOfOrdinalIgnoreCase(source, value, fromBeginning: true); } /// @@ -1139,14 +1139,14 @@ public unsafe int IndexOfNew(ReadOnlySpan source, ReadOnlySpan value /// /// contains an unsupported combination of flags. /// - public int IndexOfNew(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) + public int IndexOf(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) { Span valueAsUtf16 = stackalloc char[Rune.MaxUtf16CharsPerRune]; int charCount = value.EncodeToUtf16(valueAsUtf16); - return IndexOfNew(source, valueAsUtf16.Slice(0, charCount), options); + return IndexOf(source, valueAsUtf16.Slice(0, charCount), options); } - internal static int IndexOfOrdinalIgnoreCaseNew(ReadOnlySpan source, ReadOnlySpan value, bool fromBeginning) + internal static int IndexOfOrdinalIgnoreCase(ReadOnlySpan source, ReadOnlySpan value, bool fromBeginning) { if (value.IsEmpty) { @@ -1234,7 +1234,7 @@ internal unsafe int IndexOf(ReadOnlySpan source, ReadOnlySpan value, goto OrdinalReturn; ReturnOrdinalIgnoreCase: - retVal = IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning); + retVal = IndexOfOrdinalIgnoreCase(source, value, fromBeginning); goto OrdinalReturn; OrdinalReturn: @@ -1322,7 +1322,7 @@ public int LastIndexOf(string source, char value, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source); } - return LastIndexOfNew(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + return LastIndexOf(source, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); } public int LastIndexOf(string source, string value, CompareOptions options) @@ -1336,7 +1336,7 @@ public int LastIndexOf(string source, string value, CompareOptions options) ThrowHelper.ThrowArgumentNullException(ExceptionArgument.value); } - return LastIndexOfNew(source, value, options); + return LastIndexOf(source.AsSpan(), value.AsSpan(), options); } public int LastIndexOf(string source, char value, int startIndex) @@ -1417,7 +1417,7 @@ public int LastIndexOf(string source, char value, int startIndex, int count, Com ThrowHelper.ThrowCountArgumentOutOfRange_ArgumentOutOfRange_Count(); } - int retVal = LastIndexOfNew(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); + int retVal = LastIndexOf(sourceSpan, MemoryMarshal.CreateReadOnlySpan(ref value, 1), options); if (retVal >= 0) { retVal += startIndex; @@ -1477,7 +1477,7 @@ public int LastIndexOf(string source, string value, int startIndex, int count, C ThrowHelper.ThrowCountArgumentOutOfRange_ArgumentOutOfRange_Count(); } - int retVal = LastIndexOfNew(sourceSpan, value, options); + int retVal = LastIndexOf(sourceSpan, value, options); if (retVal >= 0) { retVal += startIndex; @@ -1498,7 +1498,7 @@ public int LastIndexOf(string source, string value, int startIndex, int count, C /// /// contains an unsupported combination of flags. /// - public unsafe int LastIndexOfNew(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) + public unsafe int LastIndexOf(ReadOnlySpan source, ReadOnlySpan value, CompareOptions options = CompareOptions.None) { if ((options & ValidIndexMaskOffFlags) == 0) { @@ -1551,7 +1551,7 @@ public unsafe int LastIndexOfNew(ReadOnlySpan source, ReadOnlySpan v return source.LastIndexOf(value); ReturnOrdinalIgnoreCase: - return IndexOfOrdinalIgnoreCaseNew(source, value, fromBeginning: false); + return IndexOfOrdinalIgnoreCase(source, value, fromBeginning: false); } /// @@ -1567,11 +1567,11 @@ public unsafe int LastIndexOfNew(ReadOnlySpan source, ReadOnlySpan v /// /// contains an unsupported combination of flags. /// - public unsafe int LastIndexOfNew(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) + public unsafe int LastIndexOf(ReadOnlySpan source, Rune value, CompareOptions options = CompareOptions.None) { Span valueAsUtf16 = stackalloc char[Rune.MaxUtf16CharsPerRune]; int charCount = value.EncodeToUtf16(valueAsUtf16); - return LastIndexOfNew(source, valueAsUtf16.Slice(0, charCount), options); + return LastIndexOf(source, valueAsUtf16.Slice(0, charCount), options); } private static int LastIndexOfOrdinal(string source, string value, int startIndex, int count, bool ignoreCase) diff --git a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs index d498ef42f37a10..3e7ddb31004be3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs +++ b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs @@ -142,15 +142,15 @@ ref MemoryMarshal.GetReference(value), { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.IndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.IndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.IndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.IndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); default: Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); - return CompareInfo.IndexOfOrdinalIgnoreCaseNew(span, value, fromBeginning: true); + return CompareInfo.IndexOfOrdinalIgnoreCase(span, value, fromBeginning: true); } } @@ -177,15 +177,15 @@ ref MemoryMarshal.GetReference(value), { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.LastIndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.LastIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.LastIndexOfNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.LastIndexOf(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); default: Debug.Assert(comparisonType == StringComparison.OrdinalIgnoreCase); - return CompareInfo.IndexOfOrdinalIgnoreCaseNew(span, value, fromBeginning: false); + return CompareInfo.IndexOfOrdinalIgnoreCase(span, value, fromBeginning: false); } } @@ -307,11 +307,11 @@ public static bool EndsWith(this ReadOnlySpan span, ReadOnlySpan val { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.IsSuffixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.IsSuffix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.IsSuffixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.IsSuffix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.Ordinal: return span.EndsWith(value); @@ -346,11 +346,11 @@ public static bool StartsWith(this ReadOnlySpan span, ReadOnlySpan v { case StringComparison.CurrentCulture: case StringComparison.CurrentCultureIgnoreCase: - return CultureInfo.CurrentCulture.CompareInfo.IsPrefixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CultureInfo.CurrentCulture.CompareInfo.IsPrefix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.InvariantCulture: case StringComparison.InvariantCultureIgnoreCase: - return CompareInfo.Invariant.IsPrefixNew(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); + return CompareInfo.Invariant.IsPrefix(span, value, string.GetCaseCompareOfComparisonCulture(comparisonType)); case StringComparison.Ordinal: return span.StartsWith(value); diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index cf9b862b8252e6..7b20000cea3e4c 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -5770,18 +5770,18 @@ internal CompareInfo() { } public int IndexOf(string source, string value, int startIndex, System.Globalization.CompareOptions options) { throw null; } public int IndexOf(string source, string value, int startIndex, int count) { throw null; } public int IndexOf(string source, string value, int startIndex, int count, System.Globalization.CompareOptions options) { throw null; } - public int IndexOfNew(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } - public int IndexOfNew(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int IndexOf(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int IndexOf(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public bool IsPrefix(string source, string prefix) { throw null; } public bool IsPrefix(string source, string prefix, System.Globalization.CompareOptions options) { throw null; } - public bool IsPrefixNew(System.ReadOnlySpan source, System.ReadOnlySpan prefix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public bool IsPrefix(System.ReadOnlySpan source, System.ReadOnlySpan prefix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public static bool IsSortable(char ch) { throw null; } public static bool IsSortable(System.ReadOnlySpan text) { throw null; } public static bool IsSortable(string text) { throw null; } public static bool IsSortable(System.Text.Rune value) { throw null; } public bool IsSuffix(string source, string suffix) { throw null; } public bool IsSuffix(string source, string suffix, System.Globalization.CompareOptions options) { throw null; } - public bool IsSuffixNew(System.ReadOnlySpan source, System.ReadOnlySpan suffix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public bool IsSuffix(System.ReadOnlySpan source, System.ReadOnlySpan suffix, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int LastIndexOf(string source, char value) { throw null; } public int LastIndexOf(string source, char value, System.Globalization.CompareOptions options) { throw null; } public int LastIndexOf(string source, char value, int startIndex) { throw null; } @@ -5794,8 +5794,8 @@ internal CompareInfo() { } public int LastIndexOf(string source, string value, int startIndex, System.Globalization.CompareOptions options) { throw null; } public int LastIndexOf(string source, string value, int startIndex, int count) { throw null; } public int LastIndexOf(string source, string value, int startIndex, int count, System.Globalization.CompareOptions options) { throw null; } - public int LastIndexOfNew(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } - public int LastIndexOfNew(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int LastIndexOf(System.ReadOnlySpan source, System.ReadOnlySpan value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int LastIndexOf(System.ReadOnlySpan source, System.Text.Rune value, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } void System.Runtime.Serialization.IDeserializationCallback.OnDeserialization(object sender) { } public override string ToString() { throw null; } } From 3d1f14bcd7d32ab150fe1d6985301f207658363c Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Sat, 11 Apr 2020 13:35:15 -0700 Subject: [PATCH 19/30] Remove spurious WindowsRuntime reference --- .../System.Private.CoreLib/src/System/String.Manipulation.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 927959e77c9a72..2fe2781e2b71b8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -6,7 +6,6 @@ using System.Diagnostics; using System.Globalization; using System.Numerics; -using System.Runtime.InteropServices.WindowsRuntime; using System.Text; using Internal.Runtime.CompilerServices; From e4ceb1835594eb0271f2a42c9dc5e8b0dce24c4c Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 14 Apr 2020 18:36:04 -0700 Subject: [PATCH 20/30] Remove outdated CoreFx.Private.TestUtilities.Unicode references --- .../Tools/GenUnicodeProp/GenUnicodeProp.csproj | 2 +- src/libraries/System.Globalization/System.Globalization.sln | 3 +++ .../tests/NlsTests/System.Globalization.Nls.Tests.csproj | 2 +- .../tests/NlsTests/System.Runtime.Nls.Tests.csproj | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/Tools/GenUnicodeProp/GenUnicodeProp.csproj b/src/coreclr/src/System.Private.CoreLib/Tools/GenUnicodeProp/GenUnicodeProp.csproj index c40c22dbb8168d..a3fde0fd8eab28 100644 --- a/src/coreclr/src/System.Private.CoreLib/Tools/GenUnicodeProp/GenUnicodeProp.csproj +++ b/src/coreclr/src/System.Private.CoreLib/Tools/GenUnicodeProp/GenUnicodeProp.csproj @@ -7,7 +7,7 @@ - + diff --git a/src/libraries/System.Globalization/System.Globalization.sln b/src/libraries/System.Globalization/System.Globalization.sln index 36e21495107ba7..c4824927738b48 100644 --- a/src/libraries/System.Globalization/System.Globalization.sln +++ b/src/libraries/System.Globalization/System.Globalization.sln @@ -26,6 +26,9 @@ EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{2E666815-2EDB-464B-9DF6-380BF4789AD4}" EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Globalization.Nls.Tests", "tests\NlsTests\System.Globalization.Nls.Tests.csproj", "{6B71E284-DA57-4657-9E08-A02BAFB877CC}" + ProjectSection(ProjectDependencies) = postProject + {2395E8CA-73CB-40DF-BE40-A60BC189B737} = {2395E8CA-73CB-40DF-BE40-A60BC189B737} + EndProjectSection EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TestUtilities", "..\Common\tests\TestUtilities\TestUtilities.csproj", "{443745F1-A200-432B-A666-3D0E9F938DED}" EndProject diff --git a/src/libraries/System.Globalization/tests/NlsTests/System.Globalization.Nls.Tests.csproj b/src/libraries/System.Globalization/tests/NlsTests/System.Globalization.Nls.Tests.csproj index 7ab807472ea6de..3266b52e575a20 100644 --- a/src/libraries/System.Globalization/tests/NlsTests/System.Globalization.Nls.Tests.csproj +++ b/src/libraries/System.Globalization/tests/NlsTests/System.Globalization.Nls.Tests.csproj @@ -340,6 +340,6 @@ - + diff --git a/src/libraries/System.Runtime/tests/NlsTests/System.Runtime.Nls.Tests.csproj b/src/libraries/System.Runtime/tests/NlsTests/System.Runtime.Nls.Tests.csproj index fa46b81ababd85..48a9940abd8ff3 100644 --- a/src/libraries/System.Runtime/tests/NlsTests/System.Runtime.Nls.Tests.csproj +++ b/src/libraries/System.Runtime/tests/NlsTests/System.Runtime.Nls.Tests.csproj @@ -56,6 +56,6 @@ - + From c34a329df8c930914e96c953821ab4b90aaab6d2 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 20 Apr 2020 16:36:51 -0700 Subject: [PATCH 21/30] PR feedback --- .../System/MemoryExtensions.Globalization.cs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs index 3e7ddb31004be3..4974928afd7036 100644 --- a/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs +++ b/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs @@ -324,13 +324,11 @@ public static bool EndsWith(this ReadOnlySpan span, ReadOnlySpan val [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static bool EndsWithOrdinalIgnoreCase(this ReadOnlySpan span, ReadOnlySpan value) - { - return value.Length <= span.Length - && CompareInfo.EqualsOrdinalIgnoreCase( - ref Unsafe.Add(ref MemoryMarshal.GetReference(span), span.Length - value.Length), - ref MemoryMarshal.GetReference(value), - value.Length); - } + => value.Length <= span.Length + && CompareInfo.EqualsOrdinalIgnoreCase( + ref Unsafe.Add(ref MemoryMarshal.GetReference(span), span.Length - value.Length), + ref MemoryMarshal.GetReference(value), + value.Length); /// /// Determines whether the beginning of the matches the specified when compared using the specified option. @@ -363,10 +361,8 @@ public static bool StartsWith(this ReadOnlySpan span, ReadOnlySpan v [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static bool StartsWithOrdinalIgnoreCase(this ReadOnlySpan span, ReadOnlySpan value) - { - return value.Length <= span.Length - && CompareInfo.EqualsOrdinalIgnoreCase(ref MemoryMarshal.GetReference(span), ref MemoryMarshal.GetReference(value), value.Length); - } + => value.Length <= span.Length + && CompareInfo.EqualsOrdinalIgnoreCase(ref MemoryMarshal.GetReference(span), ref MemoryMarshal.GetReference(value), value.Length); /// /// Returns an enumeration of from the provided span. From 880fb633956aafbd7ddcd49f48328f0d3b69840f Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 20 Apr 2020 18:17:20 -0700 Subject: [PATCH 22/30] Fix buffer overrun in ICU --- .../Unix/System.Globalization.Native/pal_collation.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c index 2ffb5f5d3a1ee1..eb39787dbeb014 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c @@ -821,14 +821,16 @@ static int32_t ComplexEndsWith(const UCollator* pCollator, UErrorCode* pErrorCod int32_t idx = usearch_last(pSearch, pErrorCode); if (idx != USEARCH_DONE) { - if ((idx + usearch_getMatchedLength(pSearch)) == patternLength) + int32_t matchEnd = idx + usearch_getMatchedLength(pSearch); + assert(matchEnd <= textLength); + + if (matchEnd == textLength) { - result = TRUE; + return TRUE; } else { - int32_t matchEnd = idx + usearch_getMatchedLength(pSearch); - int32_t remainingStringLength = patternLength - matchEnd; + int32_t remainingStringLength = textLength - matchEnd; result = CanIgnoreAllCollationElements(pCollator, pText + matchEnd, remainingStringLength); } From fc1fff3c041e77b4e34db6262ca33bd086d6d51f Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 20 Apr 2020 18:59:13 -0700 Subject: [PATCH 23/30] Fix weightless comparison bug in ICU EndsWith --- .../System/Globalization/CompareInfo.Icu.cs | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs index ca067baac33365..06cff28eb8f4df 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs @@ -632,10 +632,29 @@ private unsafe bool EndsWithOrdinalIgnoreCaseHelper(ReadOnlySpan source, R continue; } + // The match may be affected by special character. Verify that the preceding character is regular ASCII. + if (a > ap && *(a - 1) >= 0x80) + goto InteropCall; + if (b > bp && *(b - 1) >= 0x80) + goto InteropCall; + return false; + } + + // The match may be affected by special character. Verify that the preceding character is regular ASCII. + + if (source.Length < suffix.Length) + { + if (*b >= 0x80) + goto InteropCall; return false; } - return (source.Length >= suffix.Length); + if (source.Length > suffix.Length) + { + if (*a >= 0x80) + goto InteropCall; + } + return true; InteropCall: return Interop.Globalization.EndsWith(_sortHandle, bp, suffix.Length, ap, source.Length, options); @@ -672,10 +691,29 @@ private unsafe bool EndsWithOrdinalHelper(ReadOnlySpan source, ReadOnlySpa continue; } + // The match may be affected by special character. Verify that the preceding character is regular ASCII. + if (a > ap && *(a - 1) >= 0x80) + goto InteropCall; + if (b > bp && *(b - 1) >= 0x80) + goto InteropCall; + return false; + } + + // The match may be affected by special character. Verify that the preceding character is regular ASCII. + + if (source.Length < suffix.Length) + { + if (*b >= 0x80) + goto InteropCall; return false; } - return (source.Length >= suffix.Length); + if (source.Length > suffix.Length) + { + if (*a >= 0x80) + goto InteropCall; + } + return true; InteropCall: return Interop.Globalization.EndsWith(_sortHandle, bp, suffix.Length, ap, source.Length, options); From 53c83ae083bd2661a0818d6ce4b42fc017d8d1c8 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 20 Apr 2020 19:33:14 -0700 Subject: [PATCH 24/30] Account for LastIndexOf differences between NLS and ICU --- .../tests/CompareInfo/CompareInfoTests.LastIndexOf.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs index 971580b216c8e9..38bb08b1904417 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs @@ -18,6 +18,8 @@ public class CompareInfoLastIndexOfTests public static IEnumerable LastIndexOf_TestData() { + bool useNls = PlatformDetection.IsNlsGlobalization; + // Empty strings yield return new object[] { s_invariantCompare, "foo", "", 2, 3, CompareOptions.None, 3 }; yield return new object[] { s_invariantCompare, "", "", 0, 0, CompareOptions.None, 0 }; @@ -78,9 +80,11 @@ public static IEnumerable LastIndexOf_TestData() yield return new object[] { s_invariantCompare, "o\u0308", "o", 1, 2, CompareOptions.None, -1 }; // Weightless characters + // NLS matches weightless characters at the end of the string + // ICU matches weightless characters at 1 index prior to the end of the string yield return new object[] { s_invariantCompare, "", "\u200d", 0, 0, CompareOptions.None, 0 }; yield return new object[] { s_invariantCompare, "", "\u200d", -1, 0, CompareOptions.None, 0 }; - yield return new object[] { s_invariantCompare, "hello", "\u200d", 4, 5, CompareOptions.IgnoreCase, 5 }; + yield return new object[] { s_invariantCompare, "hello", "\u200d", 4, 5, CompareOptions.IgnoreCase, useNls ? 5 : 4 }; // Ignore symbols yield return new object[] { s_invariantCompare, "More Test's", "Tests", 10, 11, CompareOptions.IgnoreSymbols, 5 }; From 890fe286a087816dfb9749d094a11bf9ef37cced Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 21 Apr 2020 13:29:25 -0700 Subject: [PATCH 25/30] Fix implicit null string to empty span conversion in tests --- .../tests/CompareInfo/CompareInfoTests.Compare.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs index 4aa97c8e190228..38f884c94bfb10 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs @@ -208,6 +208,13 @@ public static IEnumerable Compare_TestData() yield return new object[] { s_invariantCompare, "Test's", null, CompareOptions.None, 1 }; yield return new object[] { s_invariantCompare, null, null, CompareOptions.None, 0 }; + yield return new object[] { s_invariantCompare, "", "Tests", CompareOptions.None, -1 }; + yield return new object[] { s_invariantCompare, "Tests", "", CompareOptions.None, 1 }; + + yield return new object[] { s_invariantCompare, null, "", CompareOptions.None, -1 }; + yield return new object[] { s_invariantCompare, "", null, CompareOptions.None, 1 }; + yield return new object[] { s_invariantCompare, "", "", CompareOptions.None, 0 }; + yield return new object[] { s_invariantCompare, new string('a', 5555), new string('a', 5555), CompareOptions.None, 0 }; yield return new object[] { s_invariantCompare, "foobar", "FooB\u00C0R", CompareOptions.IgnoreNonSpace | CompareOptions.IgnoreCase, 0 }; yield return new object[] { s_invariantCompare, "foobar", "FooB\u00C0R", CompareOptions.IgnoreNonSpace, -1 }; @@ -365,8 +372,12 @@ public void Compare_Advanced(CompareInfo compareInfo, string string1, int offset Assert.Equal(-expected, Math.Sign(compareInfo.Compare(string2, offset2, length2, string1, offset1, length1, options))); // Now test the span-based versions - use BoundedMemory to detect buffer overruns + // We can't run this test for null inputs since they implicitly convert to empty span - RunSpanCompareTest(compareInfo, string1.AsSpan(offset1, length1), string2.AsSpan(offset2, length2), options, expected); + if (string1 != null && string2 != null) + { + RunSpanCompareTest(compareInfo, string1.AsSpan(offset1, length1), string2.AsSpan(offset2, length2), options, expected); + } static void RunSpanCompareTest(CompareInfo compareInfo, ReadOnlySpan string1, ReadOnlySpan string2, CompareOptions options, int expected) { From 0bacd521a0f059387c058795a302130940c64903 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 21 Apr 2020 14:31:10 -0700 Subject: [PATCH 26/30] Patch memory leak in ComplexEndsWith --- .../Native/Unix/System.Globalization.Native/pal_collation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c index eb39787dbeb014..aacce97b19f2ef 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c @@ -826,7 +826,7 @@ static int32_t ComplexEndsWith(const UCollator* pCollator, UErrorCode* pErrorCod if (matchEnd == textLength) { - return TRUE; + result = TRUE; } else { From 64e531acaa6148696f266d19c16202d1ed7cde23 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 21 Apr 2020 21:40:27 -0700 Subject: [PATCH 27/30] Fix bad span test in StringTests --- src/libraries/Common/tests/Tests/System/StringTests.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 4cefdbb931ddf3..078765651b3faa 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -2516,7 +2516,15 @@ public static void EqualsTest(string s1, object obj, StringComparison comparison Assert.Equal(s1.GetHashCode(), s1.GetHashCode()); } - Assert.Equal(expected, s1.AsSpan().Equals(s2.AsSpan(), comparisonType)); + if (string.IsNullOrEmpty(s1) && string.IsNullOrEmpty(s2)) + { + // null strings are normalized to empty spans + Assert.True(s1.AsSpan().Equals(s2.AsSpan(), comparisonType)); + } + else + { + Assert.Equal(expected, s1.AsSpan().Equals(s2.AsSpan(), comparisonType)); + } } public static IEnumerable Equals_EncyclopaediaData() From 5747c454fb19509fefda37507337ffb714152cb1 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Tue, 21 Apr 2020 22:07:06 -0700 Subject: [PATCH 28/30] Work around ICU nullptr bug --- .../System.Globalization.Native/pal_collation.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c index aacce97b19f2ef..508d3a41e37ed8 100644 --- a/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c +++ b/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c @@ -445,6 +445,20 @@ int32_t GlobalizationNative_CompareString( if (U_SUCCESS(err)) { + // Workaround for https://unicode-org.atlassian.net/projects/ICU/issues/ICU-9396 + // The ucol_strcoll routine on some older versions of ICU doesn't correctly + // handle nullptr inputs. We'll play defensively and always flow a non-nullptr. + + UChar dummyChar = 0; + if (lpStr1 == NULL) + { + lpStr1 = &dummyChar; + } + if (lpStr2 == NULL) + { + lpStr2 = &dummyChar; + } + result = ucol_strcoll(pColl, lpStr1, cwStr1Length, lpStr2, cwStr2Length); } From 8647570f83db88ab274e261f94483ef2a5f06a0f Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 22 Apr 2020 14:05:34 -0700 Subject: [PATCH 29/30] Implement workaround for Win7 LCMapStringEx bug --- .../System/Globalization/CompareInfo.Nls.cs | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs index 3e530261d24e25..97ba5f467bb9c3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs @@ -110,6 +110,19 @@ private unsafe int NlsGetHashCodeOfString(ReadOnlySpan source, CompareOpti Debug.Assert(GlobalizationMode.UseNls); Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0); +#if TARGET_WINDOWS + if (!Environment.IsWindows8OrAbove) + { + // On Windows 7 / Server 2008, LCMapStringEx exhibits strange behaviors if the destination + // buffer is both non-null and too small for the required output. To prevent this from + // causing issues for us, we need to make an immutable copy of the input buffer so that + // its contents can't change between when we calculate the required sort key length and + // when we populate the sort key buffer. + + source = source.ToString(); + } +#endif + // LCMapStringEx doesn't support passing cchSrc = 0, so if given a null or empty input // we'll normalize it to an empty null-terminated string and pass -1 to indicate that // the underlying OS function should read until it encounters the null terminator. @@ -403,6 +416,19 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, message: SR.Argument_CannotBeEmptySpan); } +#if TARGET_WINDOWS + if (!Environment.IsWindows8OrAbove) + { + // On Windows 7 / Server 2008, LCMapStringEx exhibits strange behaviors if the destination + // buffer is both non-null and too small for the required output. To prevent this from + // causing issues for us, we need to make an immutable copy of the input buffer so that + // its contents can't change between when we calculate the required sort key length and + // when we populate the sort key buffer. + + source = source.ToString(); + } +#endif + uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options); // LCMapStringEx doesn't support passing cchSrc = 0, so if given an empty span @@ -423,6 +449,26 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, { Debug.Assert(pSource != null); Debug.Assert(pSortKey != null); + +#if TARGET_WINDOWS + if (!Environment.IsWindows8OrAbove) + { + // Manually check that the destination buffer is large enough to hold the full output. + // See earlier comment for reasoning. + + int requiredSortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName, + flags, + pSource, sourceLength, + null, 0, + null, null, _sortHandle); + + if (requiredSortKeyLength <= 0 || requiredSortKeyLength > sortKey.Length) + { + throw new ArgumentException(SR.Arg_ExternalException); + } + } +#endif + actualSortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName, flags, pSource, sourceLength, From 276cdd308f897f92b4ac5353ba968a3a01dbfed6 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 24 Apr 2020 11:41:07 -0700 Subject: [PATCH 30/30] Rename CompareInfo.GetSortKey output parameter to 'destination' Throw useful exception when destination buffer too small --- .../Windows/Kernel32/Interop.Globalization.cs | 2 +- .../tests/CompareInfo/CompareInfoTests.cs | 2 +- .../tests/Invariant/InvariantMode.cs | 2 +- .../System/Globalization/CompareInfo.Icu.cs | 17 +++++++---- .../Globalization/CompareInfo.Invariant.cs | 12 ++++---- .../System/Globalization/CompareInfo.Nls.cs | 30 ++++++++++++------- .../src/System/Globalization/CompareInfo.cs | 20 ++++++------- .../System.Runtime/ref/System.Runtime.cs | 2 +- 8 files changed, 51 insertions(+), 36 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs index 608073d36b0504..2aa2d6a1787df2 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs @@ -53,7 +53,7 @@ internal static unsafe partial class Kernel32 [DllImport("kernel32.dll", CharSet = CharSet.Unicode)] internal static extern int LocaleNameToLCID(string lpName, uint dwFlags); - [DllImport("kernel32.dll", CharSet = CharSet.Unicode)] + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] internal static extern int LCMapStringEx( string? lpLocaleName, uint dwMapFlags, diff --git a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs index 4fcf65a5648e3e..f2f3a0b2e45d87 100644 --- a/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs +++ b/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs @@ -393,7 +393,7 @@ unsafe static void RunSpanSortKeyTest(CompareInfo compareInfo, ReadOnlySpan(() => compareInfo.GetSortKey(sourceBoundedMemory.Span, sortKeyBoundedMemory.Span.Slice(1), options)); + Assert.Throws("destination", () => compareInfo.GetSortKey(sourceBoundedMemory.Span, sortKeyBoundedMemory.Span.Slice(1), options)); // Next, try with a destination which is perfectly sized - should succeed diff --git a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs index 65de5b440b4a40..8012d34ddf2be8 100644 --- a/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs +++ b/src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs @@ -698,7 +698,7 @@ public unsafe void TestSortKey_FromSpan(string input, CompareOptions options, st // First, validate that too short a buffer throws - Assert.Throws("sortKey", () => compareInfo.GetSortKey(input, new byte[expectedOutputBytes.Length - 1], options)); + Assert.Throws("destination", () => compareInfo.GetSortKey(input, new byte[expectedOutputBytes.Length - 1], options)); // Next, validate that using a properly-sized buffer succeeds // We'll use BoundedMemory to check for buffer overruns diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs index 06cff28eb8f4df..3cc636bb65dd5a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs @@ -750,7 +750,7 @@ private unsafe SortKey IcuCreateSortKey(string source, CompareOptions options) return new SortKey(this, source, options, keyData); } - private unsafe int IcuGetSortKey(ReadOnlySpan source, Span sortKey, CompareOptions options) + private unsafe int IcuGetSortKey(ReadOnlySpan source, Span destination, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); Debug.Assert(!GlobalizationMode.UseNls); @@ -761,16 +761,23 @@ private unsafe int IcuGetSortKey(ReadOnlySpan source, Span sortKey, int actualSortKeyLength; fixed (char* pSource = &MemoryMarshal.GetReference(source)) - fixed (byte* pDest = &MemoryMarshal.GetReference(sortKey)) + fixed (byte* pDest = &MemoryMarshal.GetReference(destination)) { - actualSortKeyLength = Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, pDest, sortKey.Length, options); + actualSortKeyLength = Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, pDest, destination.Length, options); } // The check below also handles errors due to negative values / overflow being returned. - if ((uint)actualSortKeyLength > (uint)sortKey.Length) + if ((uint)actualSortKeyLength > (uint)destination.Length) { - throw new ArgumentException(SR.Arg_ExternalException); + if (actualSortKeyLength > destination.Length) + { + ThrowHelper.ThrowArgumentException_DestinationTooShort(); + } + else + { + throw new ArgumentException(SR.Arg_ExternalException); + } } return actualSortKeyLength; diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs index f5ee11370d5541..9904655cea01e4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs @@ -270,7 +270,7 @@ private static void InvariantCreateSortKeyOrdinalIgnoreCase(ReadOnlySpan s } } - private int InvariantGetSortKey(ReadOnlySpan source, Span sortKey, CompareOptions options) + private int InvariantGetSortKey(ReadOnlySpan source, Span destination, CompareOptions options) { Debug.Assert(GlobalizationMode.Invariant); Debug.Assert((options & ValidCompareMaskOffFlags) == 0); @@ -279,20 +279,18 @@ private int InvariantGetSortKey(ReadOnlySpan source, Span sortKey, C // Using unsigned arithmetic below also checks for buffer overflow since the incoming // length is always a non-negative signed integer. - if ((uint)sortKey.Length < (uint)source.Length * sizeof(char)) + if ((uint)destination.Length < (uint)source.Length * sizeof(char)) { - throw new ArgumentException( - paramName: nameof(sortKey), - message: SR.Arg_BufferTooSmall); + ThrowHelper.ThrowArgumentException_DestinationTooShort(); } if ((options & CompareOptions.IgnoreCase) == 0) { - InvariantCreateSortKeyOrdinal(source, sortKey); + InvariantCreateSortKeyOrdinal(source, destination); } else { - InvariantCreateSortKeyOrdinalIgnoreCase(source, sortKey); + InvariantCreateSortKeyOrdinalIgnoreCase(source, destination); } return source.Length * sizeof(char); diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs index 97ba5f467bb9c3..2d96f963b1b1e5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Nls.cs @@ -401,7 +401,7 @@ private unsafe SortKey NlsCreateSortKey(string source, CompareOptions options) return new SortKey(this, source, options, keyData); } - private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, CompareOptions options) + private unsafe int NlsGetSortKey(ReadOnlySpan source, Span destination, CompareOptions options) { Debug.Assert(!GlobalizationMode.Invariant); Debug.Assert((options & ValidCompareMaskOffFlags) == 0); @@ -409,11 +409,9 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, // LCMapStringEx doesn't allow cchDest = 0 unless we're trying to query // the total number of bytes necessary. - if (sortKey.IsEmpty) + if (destination.IsEmpty) { - throw new ArgumentException( - paramName: nameof(sortKey), - message: SR.Argument_CannotBeEmptySpan); + ThrowHelper.ThrowArgumentException_DestinationTooShort(); } #if TARGET_WINDOWS @@ -445,7 +443,7 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, int actualSortKeyLength; fixed (char* pSource = &MemoryMarshal.GetReference(source)) - fixed (byte* pSortKey = &MemoryMarshal.GetReference(sortKey)) + fixed (byte* pSortKey = &MemoryMarshal.GetReference(destination)) { Debug.Assert(pSource != null); Debug.Assert(pSortKey != null); @@ -462,7 +460,12 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, null, 0, null, null, _sortHandle); - if (requiredSortKeyLength <= 0 || requiredSortKeyLength > sortKey.Length) + if (requiredSortKeyLength > destination.Length) + { + ThrowHelper.ThrowArgumentException_DestinationTooShort(); + } + + if (requiredSortKeyLength <= 0) { throw new ArgumentException(SR.Arg_ExternalException); } @@ -472,7 +475,7 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, actualSortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName, flags, pSource, sourceLength, - pSortKey, sortKey.Length, + pSortKey, destination.Length, null, null, _sortHandle); } @@ -484,10 +487,17 @@ private unsafe int NlsGetSortKey(ReadOnlySpan source, Span sortKey, // to allocate a temporary buffer large enough to hold intermediate state, // or the destination buffer being too small. - throw new ArgumentException(SR.Arg_ExternalException); + if (Marshal.GetLastWin32Error() == Interop.Errors.ERROR_INSUFFICIENT_BUFFER) + { + ThrowHelper.ThrowArgumentException_DestinationTooShort(); + } + else + { + throw new ArgumentException(SR.Arg_ExternalException); + } } - Debug.Assert(actualSortKeyLength <= sortKey.Length); + Debug.Assert(actualSortKeyLength <= destination.Length); return actualSortKeyLength; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs index 737105717fd9f2..7fb13f2b546255 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs @@ -1668,20 +1668,20 @@ private SortKey CreateSortKeyCore(string source, CompareOptions options) => /// Computes a sort key over the specified input. /// /// The text over which to compute the sort key. - /// The buffer into which to write the resulting sort key. + /// The buffer into which to write the resulting sort key bytes. /// The used for computing the sort key. - /// The number of bytes written to . + /// The number of bytes written to . /// - /// Use to query the required size of . + /// Use to query the required size of . /// It is acceptable to provide a larger-than-necessary output buffer to this method. /// /// - /// is too small to contain the resulting sort key; + /// is too small to contain the resulting sort key; /// or contains an unsupported flag; /// or cannot be processed using the desired /// under the current . /// - public int GetSortKey(ReadOnlySpan source, Span sortKey, CompareOptions options = CompareOptions.None) + public int GetSortKey(ReadOnlySpan source, Span destination, CompareOptions options = CompareOptions.None) { if ((options & ValidCompareMaskOffFlags) != 0) { @@ -1690,18 +1690,18 @@ public int GetSortKey(ReadOnlySpan source, Span sortKey, CompareOpti if (GlobalizationMode.Invariant) { - return InvariantGetSortKey(source, sortKey, options); + return InvariantGetSortKey(source, destination, options); } else { - return GetSortKeyCore(source, sortKey, options); + return GetSortKeyCore(source, destination, options); } } - private int GetSortKeyCore(ReadOnlySpan source, Span sortKey, CompareOptions options) => + private int GetSortKeyCore(ReadOnlySpan source, Span destination, CompareOptions options) => GlobalizationMode.UseNls ? - NlsGetSortKey(source, sortKey, options) : - IcuGetSortKey(source, sortKey, options); + NlsGetSortKey(source, destination, options) : + IcuGetSortKey(source, destination, options); /// /// Returns the length (in bytes) of the sort key that would be produced from the specified input. diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 222b35b54be602..80938da634b18f 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -5756,7 +5756,7 @@ internal CompareInfo() { } public int GetHashCode(string source, System.Globalization.CompareOptions options) { throw null; } public System.Globalization.SortKey GetSortKey(string source) { throw null; } public System.Globalization.SortKey GetSortKey(string source, System.Globalization.CompareOptions options) { throw null; } - public int GetSortKey(System.ReadOnlySpan source, System.Span sortKey, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } + public int GetSortKey(System.ReadOnlySpan source, System.Span destination, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int GetSortKeyLength(System.ReadOnlySpan source, System.Globalization.CompareOptions options = System.Globalization.CompareOptions.None) { throw null; } public int IndexOf(string source, char value) { throw null; } public int IndexOf(string source, char value, System.Globalization.CompareOptions options) { throw null; }