SpanHelpers use nuint rather than byte* and IntPtr #35765
Conversation
benaadams
changed the title
GrabYourPitchforks
left a comment
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
Some minor suggestions, but don't hold up this PR for them :)
| // Adapted from IndexOf(...) | ||
| [MethodImpl(MethodImplOptions.AggressiveOptimization)] | ||
| public static unsafe bool Contains(ref byte searchSpace, byte value, int length) | ||
| public static bool Contains(ref byte searchSpace, byte value, int length) |
There was a problem hiding this comment.
There are lots of casts ((nuint)length) in this method. (And remember, in 64-bit procs this is a signed extension.) Seems like ideally the 'length' parameter would be taken as a nuint instead of an int to avoid all of this.
There was a problem hiding this comment.
Maybe change the Asserts from
Debug.Assert(searchSpaceLength >= 0);
Debug.Assert(valueLength >= 0);to
Debug.Assert((nint)searchSpaceLength >= 0);
Debug.Assert((nint)valueLength >= 0);to make it still valid?
There was a problem hiding this comment.
Changed lengths to nuints
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs
Outdated
Show resolved
Hide resolved
| Unsafe.As<T, byte>(ref value), | ||
| count); | ||
| (uint)count); |
There was a problem hiding this comment.
This is moving more code to aggressive inlined callsite that is not an improvement. I think it was better as int before this change.
There was a problem hiding this comment.
Or leave this part of the change to separate PR where we can look at the impact.
There was a problem hiding this comment.
Reverted, can follow up
|
Can change the casts for now to double casts |
|
Thanks Ben! :) |
ghost
locked as resolved and limited conversation to collaborators
4 participants
/cc @GrabYourPitchforks @jkotas