Skip to content

src: add encodeInto to TextEncoder#28862

Closed
AtticusYang wants to merge 1 commit intonodejs:masterfrom
AtticusYang:new_encode_into
Closed

src: add encodeInto to TextEncoder#28862
AtticusYang wants to merge 1 commit intonodejs:masterfrom
AtticusYang:new_encode_into

Conversation

@AtticusYang
Copy link

@AtticusYang AtticusYang commented Jul 26, 2019

Add function encodeInto to TextEncoder, fix bug MessageChannel is not defined
in encodeInto.any.js.

Fixes: #28851
Refs: #26904

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

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. labels Jul 26, 2019
addaleax
addaleax reviewed Jul 26, 2019
View reviewed changes
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 26, 2019
@mscdex
Copy link
Contributor

mscdex commented Jul 26, 2019

Is this a dupe of #28860 ?

@addaleax addaleax mentioned this pull request Jul 26, 2019
Closed
2 tasks
@AtticusYang AtticusYang changed the title encoding: add encodeInto to TextEncoder src: add encodeInto to TextEncoder Jul 27, 2019
@AtticusYang
Copy link
Author

AtticusYang commented Jul 27, 2019

@addaleax thanks for your help. It is my great pleasure to meet such kind reviewer. I learned a lot from you. I am enjoying the whole process.

addaleax
addaleax approved these changes Jul 27, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

/cc @joyeecheung

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 28, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24737/

joyeecheung
joyeecheung reviewed Jul 29, 2019
View reviewed changes
joyeecheung
joyeecheung reviewed Aug 4, 2019
View reviewed changes
addaleax
addaleax reviewed Aug 5, 2019
View reviewed changes
AtticusYang
AtticusYang commented Aug 5, 2019
View reviewed changes
Copy link
Author

@AtticusYang AtticusYang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax @joyeecheung I am sure this commit only use string once. Please review, thank u. This indeed is more efficient but less readable. My way is a little ugly, I think there may be a more elegant way to implement encodeInto.

@AtticusYang AtticusYang force-pushed the new_encode_into branch 2 times, most recently from 2285635 to 34cc8b5 Compare August 6, 2019 05:00
@AtticusYang
Copy link
Author

AtticusYang commented Aug 9, 2019

@addaleax @joyeecheung I implemented it with only one copy. Is it ok? I am looking forward your reply, thank you.

addaleax
addaleax reviewed Aug 9, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This looks better in terms of performance, thank you :)

jasnell
jasnell reviewed Aug 13, 2019
View reviewed changes
vsemozhetbyt
vsemozhetbyt reviewed Aug 14, 2019
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Just some doc typos and consistency nits.

@addaleax
Copy link
Member

addaleax commented Aug 25, 2019

Ping @AtticusYang – are you still working on this? :)

@AtticusYang
src: add encodeInto to TextEncoder
91d61fb 
Add function encodeInto to TextEncoder, fix bug MessageChannel is not
defined in encodeInto.any.js.

Fixes: nodejs#28851
Refs: nodejs#26904
@AtticusYang
Copy link
Author

AtticusYang commented Sep 4, 2019

Ping @AtticusYang – are you still working on this? :)

I follow you and @jasnell @vsemozhetbyt tips and fix it, i pull a new request.

@AtticusYang
Copy link
Author

AtticusYang commented Sep 7, 2019

@addaleax , thanks for you help, I don't find an elegant and effective way to solve the question, so i want to close the pull request.

Ping @AtticusYang – are you still working on this? :)

@AtticusYang AtticusYang closed this Sep 7, 2019
@addaleax
Copy link
Member

addaleax commented Sep 7, 2019

@AtticusYang Do you mind if I open a new PR based on this one? This seemed quite close to being ready, tbh.

@AtticusYang
Copy link
Author

AtticusYang commented Sep 8, 2019 •
edited
Loading

@AtticusYang Do you mind if I open a new PR based on this one? This seemed quite close to being ready, tbh.

@addaleax Not at all. Go ahead.

addaleax added a commit to addaleax/node that referenced this pull request Sep 10, 2019
@addaleax
util: add encodeInto to TextEncoder
01698e6 
Add function encodeInto to TextEncoder, and add MessageChannel
to the encodeInto.any.js test.

Fixes: nodejs#28851
Fixes: nodejs#26904
Refs: nodejs#28862
Co-authored-by: AtticusYang <yyongtai@163.com>
@addaleax addaleax mentioned this pull request Sep 10, 2019
Closed
4 tasks
addaleax added a commit that referenced this pull request Sep 13, 2019
@addaleax
util: add encodeInto to TextEncoder
7c9ee6d 
Add function encodeInto to TextEncoder, and add MessageChannel
to the encodeInto.any.js test.

Fixes: #28851
Fixes: #26904
Refs: #28862
Co-authored-by: AtticusYang <yyongtai@163.com>

PR-URL: #29524
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2019
@addaleax @targos
util: add encodeInto to TextEncoder
11d1ef9 
Add function encodeInto to TextEncoder, and add MessageChannel
to the encodeInto.any.js test.

Fixes: #28851
Fixes: #26904
Refs: #28862
Co-authored-by: AtticusYang <yyongtai@163.com>

PR-URL: #29524
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@addaleax @BridgeAR
util: add encodeInto to TextEncoder
772a5e0 
Add function encodeInto to TextEncoder, and add MessageChannel
to the encodeInto.any.js test.

Fixes: #28851
Fixes: #26904
Refs: #28862
Co-authored-by: AtticusYang <yyongtai@163.com>

PR-URL: #29524
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments

@Trott Trott Trott left review comments

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

+1 more reviewer

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MessageChannel is not defined in encodeInto.any.js

8 participants

@AtticusYang @mscdex @nodejs-github-bot @addaleax @jasnell @Trott @joyeecheung @vsemozhetbyt