Skip to content

src: add encodeInto to TextEncoder #28862

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
Closed
wants to merge 1 commit into from
Closed

src: add encodeInto to TextEncoder #28862

wants to merge 1 commit into from

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

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.

8 participants

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