Proposal: Add ReadOnlySpan<char> overloads to CompareInfo

[ViktorHofer]

namespace System.Globalization
{
    public partial class CompareInfo : IDeserializationCallback
    {
        public int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options = CompareOptions.None);
        public int LastIndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options = CompareOptions.None);
        public int Compare(ReadOnlySpan<char> source, ReadOnlySpan<char> value, CompareOptions options = CompareOptions.None);
        public bool IsSuffix(ReadOnlySpan<char> source, ReadOnlySpan<char> suffix, CompareOptions options = CompareOptions.None);
        public bool IsPrefix(ReadOnlySpan<char> source, ReadOnlySpan<char> prefix, CompareOptions options = CompareOptions.None);
        public static bool IsSortable(ReadOnlySpan<char> text);
    }
}

Rationale

While doing some Span optimizations in Regex I noticed that the current comparison extension methods on ROS don't support comparison with a specific culture: MemoryExtensions.CompareTo(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType).

With string I can do that in different ways either by string.Compare(string, string, CultureInfo, CompareOptions); or by StringComparer.Create(CultureInfo, bool ignoreCase).Compare(string, string).

cc @tarekgh @ahsonkhan

[tarekgh]

Is there a specific reason that we excluded a CultureInfo overload in the extension method?

I don't think there was any specific reason to not expose this one. you cannot use the string for this comparison or your code is already using spans?

[tarekgh]

by the way, I believe we didn't expose any span API that takes CompareOptions either. If we are going to expose API takes culture, then we have to use CompareOptions that time.

[ViktorHofer]

I don't think there was any specific reason to not expose this one.

The current situation isn't ideal as I don't see a way to compare two strings with a specific culture and CompareOptions. In my case I need to allocate the string from the Span to do the comparison.

you cannot use the string for this comparison or your code is already using spans?

Correct, I'm switching to Span to reduce allocation.

by the way, I believe we didn't expose any span API that takes CompareOptions either. If we are going to expose API takes culture, then we have to use CompareOptions that time.

Agreed.

[tarekgh]

@ahsonkhan do you know any other workaround from the span side?

@ViktorHofer do you have the original strings that you constructed the span from in the first place? if so, I believe you can use these strings without the allocation. otherwise, I would say just file API request issue but I am not sure if we still open to introduce more new APIs in 2.1.

[ViktorHofer]

Not really in this layer where I'm operating. I will file an issue as I believe others will hit that also.

[ViktorHofer]

Reused this item for the proposal.

[ahsonkhan]

Should we also consider providing the other overloads on string like the ones that take bool and CultureInfo without CompareOptions:

public static int Compare(string strA, string strB, bool ignoreCase)
public static int Compare(string strA, string strB, bool ignoreCase, CultureInfo culture)

What about the other APIs like Start/EndsWith? Consider adding those to the proposal as well.

do you know any other workaround from the span side?

There isn't really a workaround for this that doesn't end up re-allocating the string.

[ViktorHofer]

Should we also consider providing the other overloads on string like the ones that take bool and CultureInfo without CompareOptions:

I believe just using bool ignoreCase is legacy and not needed if you have a CompareOptions enum. @tarekgh can correct me here.

[tarekgh]

I don't think we need the APIs takes the bool ignore cases. but we may need to have IndexOf/LastIndexOf overloads that take culture and CompareOptions

[ViktorHofer]

but we may need to have IndexOf/LastIndexOf overloads that take culture and CompareOptions

We currently don't have such for string. So this will be entirely new APIs?

[tarekgh]

We currently don't have such for string. So this will be entirely new APIs?

We have these exposed from CompareInfo class.

[ViktorHofer]

What about the other APIs like Start/EndsWith? Consider adding those to the proposal as well.

@tarekgh should we also add these? I see that these only have bool ignoreCase, CultureInfo culture overloads on string. Should we mimic them or can we also make use of CompareOptions here?

[tarekgh]

should we also add these? I see that these only have bool ignoreCase, CultureInfo culture overloads on string. Should we mimic them or can we also make use of CompareOptions here?

The apis should look like:

 public static int IndexOf(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, CultureInfo culture, CompareOptions options);
 public static int LastIndexOf(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, CultureInfo culture, CompareOptions options);
 public static bool IsPrefix(String, String, CultureInfo culture, CompareOptions options);
 public static bool IsSuffix(String, String, CultureInfo culture, CompareOptions options);

The other option is to add these extension methods to CompareInfo (instead of the Span) and at that time we'll not pass CultureInfo.

I prefer the CompareInfo option because this will be consistent with the strings APIs.

[tarekgh]

Also, I prefer to expose the proposed CompareTo from CompareInfo too with the name "Compare"