FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

dgram: support Uint8Array input to send() by addaleax · Pull Request #11985 · nodejs/node · GitHub

/ node Public

dgram: support Uint8Array input to send() - #11985

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:dgram-uint8array
ConversationCommits (2)ChecksFiles changed
Closed

dgram: support Uint8Array input to send()#11985
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:dgram-uint8array

Conversation

addaleax commented Mar 22, 2017

Copy link
Copy Markdown
Member

Fixes: #11954
Refs: #11961

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

addaleax added dgram Issues and PRs related to the dgram subsystem / UDP. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 22, 2017
addaleax requested a review from TimothyGu March 22, 2017 07:07
nodejs-github-bot added the dgram Issues and PRs related to the dgram subsystem / UDP. label Mar 22, 2017
jasnell approved these changes Mar 22, 2017
View reviewed changes
Comment thread lib/dgram.js Outdated

jasnell Mar 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since the error message is changing anyway, can you s/buffer/Buffer here to make it consistent with the other error messages thrown in this module?

addaleax Mar 22, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell Right, done!

addaleax commented Mar 24, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/8656/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/672/

joyeecheung reviewed Mar 24, 2017
View reviewed changes
Comment thread test/parallel/test-dgram-send-default-host.js Outdated
const port = client.address().port;
client.send(toSend[0], 0, toSend[0].length, port);
client.send(toSend[1], port);
client.send(new Uint8Array([...toSend[1]]), port);

joyeecheung Mar 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is missing test cases for socket.send(u8a, offset, length, port) and socket.send(buffer, port).

Also, is new Uint8Array([...toSend[1]]) here the same as new Uint8Array(toSend[1])?

addaleax Mar 24, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@joyeecheung Right, thanks for pointing that out. I’ve updated the file so all the tests are run twice now, once with the original input types and once with Uint8Arrays.

addaleax commented Mar 27, 2017

Copy link
Copy Markdown
Member Author

Landed in 2dc1053

addaleax closed this Mar 27, 2017
addaleax deleted the dgram-uint8array branch March 27, 2017 00:08
addaleax added a commit that referenced this pull request Mar 27, 2017
dgram: support Uint8Array input to send()
2dc1053 
Fixes: #11954
Refs: #11961
PR-URL: #11985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
jasnell mentioned this pull request Apr 4, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
+1 more reviewer
TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dgram Issues and PRs related to the dgram subsystem / UDP. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Buffer::Length hard crashes

7 participants

Back | FazBrowse Home | New Git URL