This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Span: Add BinarySearch(...) extension methods for ReadOnlySpan<T> (and Span<T>) #25777
Merged
Merged
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
d4f644a
S.M: Add initial BinarySearch span extensions method definitions.
nietras 0d76e79
Add dummy xml comments to allow compiling. Add empty test files.
nietras 9fb189f
Add to ref.
nietras 9b5ad5b
Uncomment in test since extension method not found.
nietras 8b4a8ba
Add initial implementation.
nietras cd7e6a7
Add more comments and string test.
nietras 039349c
Add more tests.
nietras 3d0ac20
Fix string test, add double tests.
nietras b84fad2
cleanup
nietras dc79fad
Remove empty check, and add empty tests.
nietras 88c8e0b
Remove xml doc on exception, since that case is not applicable here.
nietras d86b248
Remove try/catch, and SR text etc.
nietras 0a8a9c2
minor cleanup
nietras 471f1f7
refactor tests.
nietras d953a2c
remove redundant.
nietras a31def9
merge master
nietras 4327a5f
refactor string tests.
nietras 9a19658
refactor tests, add comparer tests.
nietras c68312e
add icomparable overload tests.
nietras 41ebc1c
Add test for int.MaxValue size span and possible overflow.
nietras 42c5dec
compute median without subtract
nietras 1e3b969
make generic params 'in'
nietras 74f0496
Remove 'in' and update xml comments.
nietras 5ce195e
add a few more test
nietras 6d75e2a
add null throw tests
nietras 90f3525
simplify max length no overflow test
nietras 955bc4b
add basic perf tests.
nietras ee9830f
fix remaining review issues.
nietras cedb211
use InnerCount
nietras 0b3ccec
use InnerCount
nietras dc60908
fix string perf test
nietras 67a5965
fix MiddleIndex perf tests
nietras File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// 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.Collections.Generic; | ||
using System.Runtime.CompilerServices; | ||
|
||
namespace System | ||
{ | ||
internal static partial class SpanHelpers | ||
{ | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
internal static int BinarySearch<T, TComparable>( | ||
this ReadOnlySpan<T> span, TComparable comparable) | ||
where TComparable : IComparable<T> | ||
{ | ||
if (comparable == null) | ||
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparable); | ||
// TODO: Make `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it | ||
return BinarySearch(ref span.DangerousGetPinnableReference(), span.Length, comparable); | ||
} | ||
|
||
// TODO: Make spanStart `ref readonly`/`in` when Unsafe.Add(ReadOnly) supports it | ||
internal static int BinarySearch<T, TComparable>( | ||
ref T spanStart, int length, TComparable comparable) | ||
where TComparable : IComparable<T> | ||
{ | ||
int lo = 0; | ||
int hi = length - 1; | ||
// If length == 0, hi == -1, and loop will not be entered | ||
while (lo <= hi) | ||
{ | ||
// PERF: `lo` or `hi` will never be negative inside the loop, | ||
// so computing median using uints is safe since we know | ||
// `length <= int.MaxValue`, and indices are >= 0 | ||
// and thus cannot overflow an uint. | ||
// Saves one subtraction per loop compared to | ||
// `int i = lo + ((hi - lo) >> 1);` | ||
int i = (int)(((uint)hi + (uint)lo) >> 1); | ||
|
||
// TODO: We probably need to add `ref readonly`/`in` methods e.g. `AddReadOnly` to unsafe | ||
int c = comparable.CompareTo(Unsafe.Add(ref spanStart, i)); | ||
if (c == 0) | ||
{ | ||
return i; | ||
} | ||
else if (c > 0) | ||
{ | ||
lo = i + 1; | ||
} | ||
else | ||
{ | ||
hi = i - 1; | ||
} | ||
} | ||
// If none found, then a negative number that is the bitwise complement | ||
// of the index of the next element that is larger than or, if there is | ||
// no larger element, the bitwise complement of `length`, which | ||
// is `lo` at this point. | ||
return ~lo; | ||
} | ||
|
||
// Helper to allow sharing all code via IComparable<T> inlineable | ||
internal struct ComparerComparable<T, TComparer> : IComparable<T> | ||
where TComparer : IComparer<T> | ||
{ | ||
readonly T _value; | ||
readonly TComparer _comparer; | ||
|
||
public ComparerComparable(T value, TComparer comparer) | ||
{ | ||
_value = value; | ||
_comparer = comparer; | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public int CompareTo(T other) => _comparer.Compare(_value, other); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,8 @@ internal enum ExceptionArgument | |
text, | ||
obj, | ||
ownedMemory, | ||
pointer | ||
pointer, | ||
comparable, | ||
comparer | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahsonkhan, @nietras , do we have workitems filed for these Unsafe APIs and to fix the TODOs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KrzysztofCwalina My proposal to change
Unsafe
APIs toref readonly
was rejected a while ago. A few things have changed since then (such as the rename toin
), so maybe it's time the proposal could be revisited?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found https://github.com/dotnet/corefx/issues/25814.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably just remove the TODOs since the
Unsafe
fix was to "cast" away thereadonly
and then there is no reason as such to change the impl, ifDangerousGetPinnableReference()
will still return aref
forReadOnlySpan<T>
otherwise thereadonly
simply has to be cast away. This will show up if any such change comes toReadOnlySpan<T>
so deleting the TODOs should be fine.