Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Adding Span IndexOfAny APIs that take 2 and 3 bytes #17357

Merged
merged 6 commits into from
Mar 30, 2017
Merged

Adding Span IndexOfAny APIs that take 2 and 3 bytes #17357

merged 6 commits into from
Mar 30, 2017

Conversation

ahsonkhan
Copy link

@ahsonkhan ahsonkhan commented Mar 21, 2017 •
edited
Loading

From:
dotnet/corefxlab#1348 (comment)

Part of dotnet/corefxlab#1314 / dotnet/corefxlab#1293

Adding:

    public static int IndexOfAny(this Span<byte> span, byte value0, byte value1)
    public static int IndexOfAny(this Span<byte> span, byte value0, byte value1, byte value2)
    public static int IndexOfAny(this Span<byte> span, ReadOnlySpan<byte> values)
    public static int IndexOfAny(this ReadOnlySpan<byte> span, byte value0, byte value1)
    public static int IndexOfAny(this ReadOnlySpan<byte> span, byte value0, byte value1, byte value2)
    public static int IndexOfAny(this ReadOnlySpan<byte> span, ReadOnlySpan<byte> values)

Edit: IndexOf => IndexOfAny

cc @KrzysztofCwalina, @davidfowl

@ahsonkhan ahsonkhan mentioned this pull request Mar 21, 2017
Merged
@ahsonkhan
Copy link
Author

ahsonkhan commented Mar 22, 2017 •
edited
Loading

WIP. I have to change the logic and tests.

I will be renaming these overloads to IndexOfAny.

dotnet/corefxlab#1352

dotnet/corefxlab#1354

Is that fine?

@karelz
Copy link
Member

karelz commented Mar 22, 2017 •
edited
Loading

@terrajobst @ahsonkhan how are Span API additions reviewed?

@ahsonkhan ahsonkhan changed the title Adding Span IndexOf overloads that take multiple bytes. Adding Span IndexOfAny APIs that take 2 and 3 bytes Mar 22, 2017
@ahsonkhan
Copy link
Author

cc @KrzysztofCwalina, @shiftylogic, please take a look.

@ahsonkhan ahsonkhan mentioned this pull request Mar 22, 2017
Closed
@karelz
Copy link
Member

karelz commented Mar 22, 2017 •
edited
Loading

@ahsonkhan how are the APIs in Span reviewed? (not code reviewed, but API-reviewed)

@ahsonkhan
Copy link
Author

ahsonkhan commented Mar 22, 2017 •
edited
Loading

@karelz, they were informally reviewed as part of the discussion that took place for dotnet/corefxlab#1292

@jkotas, @KrzysztofCwalina, @terrajobst, thoughts?

@jkotas
Copy link
Member

jkotas commented Mar 22, 2017

@ahsonkhan Adding new APIs is documented at https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md . We should follow it for adding new Span APIs in corefx as well.

shiftylogic
shiftylogic reviewed Mar 22, 2017
View reviewed changes
src/System.Memory/src/System/SpanHelpers.byte.cs Outdated
Vector.BitwiseAnd(
Vector.Equals(vData, values0),
Vector.Equals(vData, values1)),
Vector.Equals(vData, values2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Spacing seems off

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the spacing should look like exactly, but made a change.

src/System.Memory/src/System/SpanHelpers.byte.cs Outdated
break;
}
index += Vector<byte>.Count;
} while ((byte*) nLength > (byte*) index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove spaces after casts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ReSharper/VS auto formatted to add those spaces. Will remove.

@karelz karelz added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Mar 22, 2017
@ahsonkhan ahsonkhan mentioned this pull request Mar 23, 2017
Merged
@ahsonkhan
Copy link
Author

@dotnet-bot test Innerloop OSX10.12 Debug x64 Build and Test

@ahsonkhan
Copy link
Author

The API has been reviewed. Can this be merged now?

https://github.com/dotnet/corefx/issues/17414

@ahsonkhan ahsonkhan removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Mar 29, 2017
@ahsonkhan
Copy link
Author

ahsonkhan commented Mar 29, 2017 •
edited
Loading

cc @shiftylogic, @KrzysztofCwalina, @jkotas

@karelz
Copy link
Member

karelz commented Mar 29, 2017

We are still blocked on API approval as there have been further concerns: https://github.com/dotnet/corefx/issues/17414#issuecomment-289932452

@ahsonkhan let's not rush the API through please. I think we should wait on explicit green light from @KrzysztofCwalina and @jkotas on the API proposal. I am not sure if we have to do another API review next week.

@karelz karelz added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Mar 29, 2017
@jkotas
Copy link
Member

jkotas commented Mar 29, 2017

@karelz There are no concerns about this part of the API proposal.

shiftylogic
shiftylogic reviewed Mar 29, 2017
View reviewed changes
src/System.Memory/src/System/SpanHelpers.byte.cs Outdated
unchecked
{
int unaligned = (int)(byte*)Unsafe.AsPointer(ref searchSpace) & (Vector<byte>.Count - 1);
nLength = (IntPtr)(uint)unaligned;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this still backwards? If the purpose is to fix alignment, then you need to re-align the data on Vector.Count boundary. 'unaligned' above ends up being the number of bytes of misalignment. That would mean you need to advance Vector.Count - unaligned to get the data back into alignment.

Copy link
Author

@ahsonkhan ahsonkhan Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will propagate the changes we made earlier:
#17500

shiftylogic
shiftylogic reviewed Mar 29, 2017
View reviewed changes
src/System.Memory/src/System/SpanHelpers.byte.cs Outdated
unchecked
{
int unaligned = (int)(byte*)Unsafe.AsPointer(ref searchSpace) & (Vector<byte>.Count - 1);
nLength = (IntPtr)(uint)unaligned;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same alignment issue as above.

Copy link
Author

@ahsonkhan ahsonkhan Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I will propagate the changes we made earlier:
#17500

@KrzysztofCwalina
Copy link
Member

Looks good.

@shiftylogic
Copy link
Contributor

👍

@ahsonkhan ahsonkhan removed * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Mar 30, 2017
@ahsonkhan
Copy link
Author

@shiftylogic, good to go?

@ahsonkhan ahsonkhan merged commit 50e60c8 into dotnet:master Mar 30, 2017
@ahsonkhan ahsonkhan deleted the IndexOfOverloads branch March 30, 2017 19:05
@ahsonkhan ahsonkhan mentioned this pull request Mar 31, 2017
Closed
@karelz karelz modified the milestone: 2.0.0 Mar 31, 2017
This was referenced Mar 31, 2017
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shiftylogic shiftylogic shiftylogic approved these changes

@jkotas jkotas jkotas approved these changes

Assignees

@ahsonkhan ahsonkhan

@jkotas jkotas

@KrzysztofCwalina KrzysztofCwalina

@shiftylogic shiftylogic

Labels
area-System.Memory
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ahsonkhan @karelz @jkotas @KrzysztofCwalina @shiftylogic @dnfclas