Skip to content
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

src: add encodeInto to TextEncoder #28862

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

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
lib/internal/encoding.js Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
@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

@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

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

joyeecheung
joyeecheung reviewed Aug 4, 2019
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Aug 5, 2019
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
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.

src/node_buffer.cc Outdated Show resolved Hide resolved
@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

@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 :)

src/node_buffer.cc Outdated Show resolved Hide resolved
src/node_buffer.cc Show resolved Hide resolved
src/node_buffer.cc Show resolved Hide resolved
jasnell
jasnell reviewed Aug 13, 2019
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
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.

doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

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

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

@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

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@joyeecheung joyeecheung joyeecheung left review comments

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott left review comments

@jasnell jasnell jasnell left review comments

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