Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add notes for System.Binary #44

Merged
merged 1 commit into from
Oct 4, 2017
Merged

Add notes for System.Binary #44

merged 1 commit into from
Oct 4, 2017

Conversation

terrajobst
Copy link
Contributor

ahsonkhan
ahsonkhan reviewed Oct 3, 2017
View reviewed changes
2017/10-03-System.Binary/README.md Outdated
- Already contains the existing V1 `ArrayPool`

(1) has to be in-box
(2) doesn't have to be, but might due to `ArrayPool`. In fact, on .NET Core we

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ahsonkhan
ahsonkhan approved these changes Oct 3, 2017
View reviewed changes
Copy link
Contributor

@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.

Otherwise, LGTM.

ahsonkhan
ahsonkhan reviewed Oct 3, 2017
View reviewed changes
2017/10-03-System.Binary/README.md
* Extension methods
- If we want to expose them as extension methods we need to have both
overloads for `ReadOnlySpan<T>` and `Span<T>`
- Let's do non-extension methods for now; we can make them extension methods

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ahsonkhan
ahsonkhan reviewed Oct 3, 2017
View reviewed changes
2017/10-03-System.Binary/README.md Outdated
overloads for `ReadOnlySpan<T>` and `Span<T>`
- Let's do non-extension methods for now; we can make them extension methods
later.
+ But let's leave the type name to be `BufferPrimitives` (as opposed to

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

KrzysztofCwalina
KrzysztofCwalina reviewed Oct 3, 2017
View reviewed changes
2017/10-03-System.Binary/README.md Outdated
- Formatting & parsing
* `System.Buffers.Binary`
- The type discussed here
- `BinaryExtensions`

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ahsonkhan
ahsonkhan reviewed Oct 3, 2017
View reviewed changes
2017/10-03-System.Binary/README.md Outdated
- Should `Read<T>` return a `ref T`?
+ No, because that wouldn't guarantee any alignment. Returning a `T`
guarantees that it's aligned because we always align the stack
- Should `Write<T>` take a `ref T`?

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@terrajobst terrajobst merged commit 8c66500 into master Oct 4, 2017
@terrajobst terrajobst deleted the System.Binary branch October 4, 2017 20:36
ahsonkhan
ahsonkhan reviewed Oct 4, 2017
View reviewed changes
2017/10-03-System.Binary/README.md
1. `System.Memory`
- OOB `Span<T>`
- Later type forwarded to the core library where span is in-box
2. `System.Buffers`

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benaadams benaadams benaadams left review comments

@ahsonkhan ahsonkhan ahsonkhan approved these changes

@KrzysztofCwalina KrzysztofCwalina KrzysztofCwalina left review comments

Assignees
No one assigned
Labels
cla-already-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@terrajobst @benaadams @ahsonkhan @KrzysztofCwalina @dnfclas