GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — dgram: make UDPWrap more reusable by jasnell · Pull Request #31871 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Feb 19, 2020

Copy link
Copy Markdown
Member

Authored by @addaleax and originally landed in the QUIC repo. Separating out from the QUIC PR. While this does not have any QUIC specific bits, QUIC would be the only current use of it so it would likely be best to land this PR in conjunction with the QUIC PR. Separating out only to make it easier to review.

Allow using the handle more directly for I/O in other parts of the codebase.

Original review metadata:

  PR-URL: https://github.com/nodejs/quic/pull/165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Feb 19, 2020
addaleax
addaleax approved these changes Feb 19, 2020
View reviewed changes
mscdex
mscdex reviewed Feb 19, 2020
View reviewed changes
Comment thread lib/dgram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Feb 20, 2020
View reviewed changes
Comment thread src/udp_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/udp_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/udp_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Feb 24, 2020

Copy link
Copy Markdown
Member Author

@addaleax ... given that you wrote the original change here, do you have thoughts on the suggestions from @mscdex and @bnoordhuis ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Feb 24, 2020

Copy link
Copy Markdown
Member

@jasnell Feel free to address Ben’s comments (or if you want, I can do that later this evening)
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested review from addaleax and bnoordhuis February 26, 2020 02:54
@jasnell

jasnell commented Feb 29, 2020

Copy link
Copy Markdown
Member Author

Waiting for #31960 to land then will rebase this and update it to incorporate those changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the reusable-udpwrap branch from 744a2ce to 34721a8 Compare March 2, 2020 21:30
@jasnell

jasnell commented Mar 2, 2020

Copy link
Copy Markdown
Member Author

Rebased now that #31960 landed. @bnoordhuis and @addaleax, please take another look
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Mar 2, 2020
View reviewed changes
Comment thread src/udp_wrap.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Mar 2, 2020
View reviewed changes
@jasnell jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 2, 2020
@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29503/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax @jasnell
dgram: make UDPWrap more reusable
347ea10 
Allow using the handle more directly for I/O in other parts of
the codebase.

Originally landed in the QUIC repo

Original review metadata:

```
  PR-URL: nodejs/quic#165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
```

Signed-off-by: James M Snell <[email protected]>
@jasnell
jasnell force-pushed the reusable-udpwrap branch from 34721a8 to 347ea10 Compare March 2, 2020 22:34
@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29506/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
fixup! dgram: make UDPWrap more reusable
fe77e7c
bnoordhuis
bnoordhuis approved these changes Mar 3, 2020
View reviewed changes
jasnell pushed a commit that referenced this pull request Mar 3, 2020
@addaleax @jasnell
dgram: make UDPWrap more reusable
43b7142 
Allow using the handle more directly for I/O in other parts of
the codebase.

Originally landed in the QUIC repo

Original review metadata:

```
  PR-URL: nodejs/quic#165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
```

Signed-off-by: James M Snell <[email protected]>

PR-URL: #31871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@jasnell

jasnell commented Mar 3, 2020

Copy link
Copy Markdown
Member Author

Landed in 43b7142
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Mar 3, 2020
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@addaleax @MylesBorins
dgram: make UDPWrap more reusable
4dc59b9 
Allow using the handle more directly for I/O in other parts of
the codebase.

Originally landed in the QUIC repo

Original review metadata:

```
  PR-URL: nodejs/quic#165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
```

Signed-off-by: James M Snell <[email protected]>

PR-URL: #31871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
v13.10.1 proposal #32099
Merged
@targos targos added the backport-blocked-v12.x label Apr 20, 2020
@targos

targos commented Apr 20, 2020

Copy link
Copy Markdown
Member

Depends on #31960 to land on v12.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos mentioned this pull request Apr 20, 2020
src: introduce node_sockaddr #32070
Closed
3 tasks done
@targos targos removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 25, 2020
@targos targos removed the backport-blocked-v12.x label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax @targos
dgram: make UDPWrap more reusable
ea17cf6 
Allow using the handle more directly for I/O in other parts of
the codebase.

Originally landed in the QUIC repo

Original review metadata:

```
  PR-URL: nodejs/quic#165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
```

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#31871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @targos
dgram: make UDPWrap more reusable
322a998 
Allow using the handle more directly for I/O in other parts of
the codebase.

Originally landed in the QUIC repo

Original review metadata:

```
  PR-URL: nodejs/quic#165
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Daniel Bevenius <[email protected]>
```

Signed-off-by: James M Snell <[email protected]>

PR-URL: #31871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @addaleax @nodejs-github-bot @targos @mscdex @bnoordhuis
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.