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

Implement SequenceReader.TryPeek(long offset, out T value) #42364

Merged
merged 4 commits into from
Nov 7, 2019
Merged

Implement SequenceReader.TryPeek(long offset, out T value) #42364

merged 4 commits into from
Nov 7, 2019

Conversation

MarcoRossignoli
Copy link
Member

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! I'd optimize for peeking within the current span, then fall back to using the internal TryCopyMultisegment as discussed inline.

The key design point is to make any in span operations as fast as possible. When we cross spans we should still try to be efficient, but it is okay to not be micro-optimized and favor code-reuse / clarity.

@MarcoRossignoli MarcoRossignoli changed the title Implement trypeek offset Implement SequenceReader.TryPeek(long offset, out T value) Nov 6, 2019
@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your corefx repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

@MarcoRossignoli MarcoRossignoli marked this pull request as ready for review November 6, 2019 21:56
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Sorry about the unnecessary confusion. Looks good with just some minor issues with comments (notably capitalizing the first word). You should also be validating that we get default out when the method returns false.

@MarcoRossignoli
Copy link
Member Author

@JeremyKuhne ready to review, I'll take some other api after that.

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks!

@JeremyKuhne JeremyKuhne merged commit 43d30a1 into dotnet:master Nov 7, 2019
@davidfowl
Copy link
Member

Awesome!

@MarcoRossignoli MarcoRossignoli deleted the trypeekoffset branch November 7, 2019 18:55
@ahsonkhan ahsonkhan added this to the 5.0 milestone Nov 14, 2019
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Some late review nits.

@MarcoRossignoli
Copy link
Member Author

MarcoRossignoli commented Nov 14, 2019

thank's @ahsonkhan for review...bit busy days I'll take a look starting in middle of next week if it's not a problem, it's on my backlog now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants