Renaming IndexOfVectorized to IndexOf#1320
Conversation
| } | ||
|
|
||
| public unsafe static int IndexOfVectorized(this Span<byte> buffer, byte value) | ||
| public static int IndexOfNon_Vectorized(this Span<byte> span, byte value) |
There was a problem hiding this comment.
It should be without underscore, I think
|
Is conveying implementation detail? Maybe |
|
What about calling it IndexOfSequential? or SequentialIndexOf. |
| length -= 8; | ||
|
|
||
| if (value == Unsafe.Add(ref searchSpace, index)) | ||
| goto Found; |
There was a problem hiding this comment.
Should this be called Found0 for consistency?
There was a problem hiding this comment.
Sure, but I grabbed the code from corefx and it has goto Found in many places:
Should I fix this in both repos?
There was a problem hiding this comment.
I have intentionally named it this way: there is index and no index + 0 here either; and Found0 looked odd in the last loop that goes by single element.
| if (length < Vector<byte>.Count) | ||
| Debug.Assert(length >= 0); | ||
|
|
||
| IntPtr index = (IntPtr)0; // Use IntPtr for arithmetic to avoid unnecessary 64->32->64 truncations |
There was a problem hiding this comment.
I would use IntPtr.Zero
There was a problem hiding this comment.
Similar to above, should this be fixed everywhere?
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/SpanHelpers.T.cs#L53
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/SpanHelpers.byte.cs#L55
There was a problem hiding this comment.
There are many hundreds instances of both forms in corefx. (My rule of thumb - if there are two equivalent forms, use the one that has fewer characters.)
There was a problem hiding this comment.
In dotnet/coreclr#9473 I found IntPtr.Zero to generally be worse as it does a memory load as a readonly static struct rather than a conversion that is then elided.
Is noted as the reason for the existence of internal IsNull()
There was a problem hiding this comment.
IntPtr.Zero is treated as intrinsic by the JIT and should be JITed as zero constant. Look for CORINFO_FIELD_INTRINSIC_ZERO with optimizations on. If you do not see it happening, it is a bug that should be fixed.
The comment on IsNull predates the CORINFO_FIELD_INTRINSIC_ZERO optimization.
There was a problem hiding this comment.
Seems like we should do the cleanup separately (if we even want).
There was a problem hiding this comment.
Will have a look at it again and raise issue if I come across it again (I think it was less happy to inline using IntPtr.Zero)
|
looks good. |
5 participants
IndexOfNon_VectorizedSequentialIndexOf (for comparison).@KrzysztofCwalina, is the method name fine?
cc @davidfowl