Implement Span LastIndexOf extension method and add tests#25748
Conversation
| public static int IndexOfAny(this ReadOnlySpan<byte> span, byte value0, byte value1, byte value2) { throw null; } | ||
| public static int IndexOfAny(this ReadOnlySpan<byte> span, ReadOnlySpan<byte> values) { throw null; } | ||
|
|
||
| public static int LastIndexOf<T>(this ReadOnlySpan<T> span, T value) where T : IEquatable<T> { throw null; } |
There was a problem hiding this comment.
Should we also have LastIndexOfAny for parity?
There was a problem hiding this comment.
Yes. I will add those in a later PR.
There was a problem hiding this comment.
Could you file an issue? Thanks.
| int valueTailLength = valueLength - 1; | ||
|
|
||
| int index = 0; | ||
| for (;;) |
There was a problem hiding this comment.
So if span is "abaaaaaaaaaaa..." and I search for "ab", it will do lots of loops. Maybe there is nothing we can do.
There was a problem hiding this comment.
The same would apply to something like "...aaaaaaaaab" and you search for "ab" when you call IndexOf (which is where I borrowed the template for LastIndexOf). There may be a different approaches that balance things better but I haven't thought too deeply about it yet since it would require a bigger change (to all our IndexOf APIs).
|
|
||
| using Xunit; | ||
|
|
||
| namespace System.SpanTests |
There was a problem hiding this comment.
It might be worth adding tests that use a reference type T implementing IEquatable. I think all the current tests would work regardless of the code using IEquatable or simply comparing bits in the structs.
There was a problem hiding this comment.
Added tests for {RO}Span<string>
There was a problem hiding this comment.
Well, it's better than just testing the structs, but unfortunately String.Equals(string) does ordinal comparison, i.e. compares bits in the string :-). Maybe it's ok though.
Also, I just realized we should probably add LastIndexOf(T, Comparer) to let people chose whatever string comparison they want (not this PR of course).
cc @davidsh |
|
Is this good to merge? |
|
It looks good to me. Also, thanks for all the cleanup, though next time it might be better to have two PRs: one for cleanup and one for the feature. |
…efx#25748) * Implement Span LastIndexOf and add tests * Add LastIndexOfSequence tests and fix implementation * Vectorize LastIndexOf<byte> similar to IndexOf<byte> * Add LastIndexOf performance tests * Adding IndexOf and LastIndexOf tests for reference type (string). * Use abbreviated version of 'default' literal * Remove unnecessary type specifiers in generic function calls. * Cleanup and format all files in the solution to follow coding style. * Cleaning up leftover unused using directives and extra spaces Commit migrated from dotnet/corefx@d219378
3 participants
Part of https://github.com/dotnet/corefx/issues/24839
Following IndexOf vectorization from this PR: #17143
Left to do: LastIndexOfAny APIs
Also did a lot of cleaning and code formatting.
cc @benaadams, @KrzysztofCwalina, @GrabYourPitchforks, @jkotas, @stephentoub