Skip to content

Conversation

@geoffkizer
Copy link
Contributor

Fixes #45267

@antonfirsov @stephentoub @dotnet/ncl

@ghost
Copy link

ghost commented Jan 14, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 14, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #45267

@antonfirsov @stephentoub @dotnet/ncl

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Sockets, new-api-needs-documentation

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

A few concerns regarding tests, otherwise looks good.

I would add at least one test that uses NativeMemoryManager to wrap an actual pointer into ReadOnlyMemory<byte> & apply custom some offset + length there to stress that behavior too.

@geoffkizer
Copy link
Contributor Author

There are also a bunch of tests marked as [Outerloop] that don't need to be, as far as I can tell. Will remove this.

@geoffkizer
Copy link
Contributor Author

Feedback addressed. Please take a look.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

return new ElementWithMemoryManager(new SendPacketsElement(memoryManager.Memory.Slice(offset, count)), memoryManager);
}

private static ElementWithMemoryManager CreateElementForBuffer(BufferType bufferType, int size) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@geoffkizer geoffkizer merged commit 059b17e into dotnet:master Jan 15, 2021
@geoffkizer geoffkizer deleted the sendpacketelementsmemory branch January 15, 2021 17:57
@geoffkizer geoffkizer mentioned this pull request Jan 15, 2021
14 tasks
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Memory support to SendPacketElements

4 participants