SpanHelpers use nuint rather than byte* and IntPtr#35765
SpanHelpers use nuint rather than byte* and IntPtr#35765jkotas merged 3 commits intodotnet:masterfrom
Conversation
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
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! :) |
/cc @GrabYourPitchforks @jkotas