Extract base64 encoding/decoding code into headers by eugeneo · Pull Request #6910 · nodejs/node

eugeneo · 2016-05-21T00:38:23Z

This is related to PR #6792

For our protocol, we need Base64 encoder. It already exists in string_bytes.cc so I simply extracted it to the header. For symmetry, I also extracted decoder.

mscdex · 2016-05-21T01:46:13Z

/cc @nodejs/ctc Is this an API we want to support? I'm leaning towards -1 but more like -0.5 I guess....

EDIT: Thinking about this more I move my vote to -1 ;-)

indutny · 2016-05-21T01:51:26Z

-1 from me as well, this is an implementation detail, not an API.

ofrobots · 2016-05-21T05:20:05Z

This looks like a refactoring of code so base64 could be used in other parts of Node core. This is not a new API. Did I miss something?

ofrobots · 2016-05-21T05:22:48Z

@eugeneo The commit message should be formatted according to the commit guidelines: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

mscdex · 2016-05-21T05:33:16Z

@ofrobots This PR moves the code from the existing string_bytes.cc to a new header file which is includable from outside of node, effectively making it a (newly) exposed API.

ofrobots · 2016-05-21T05:42:34Z

But I could use the same 'API' by including string_bytes.cc from outside of node. It almost sounds like you are saying that if the refactored file was renamed to be base64.c it would be okay? How does one reuse C++ code in core without creating a header file?

indutny · 2016-05-21T07:16:00Z

@ofrobots this doesn't really feat into the concept of API, IMO. Node.js is about JS VM, OS bindings, and the rest. base64 is clearly not something that node offers by itself. While we carry it right now, it doesn't mean that we will do it this way forever, and I don't want to lock us in this kind of API.

bnoordhuis · 2016-05-21T09:09:17Z

Can you leave the table itself in string_bytes.cc? Just drop the static keyword and declare it here. Maybe add [256] so there is no confusion about its size.

bnoordhuis · 2016-05-21T09:14:26Z

LGTM with a comment and the same note about the commit log that @ofrobots mentioned.

All: this is moving internal code around for the benefit of #6792. If people are dumb enough to include it in their own code, they get what they deserve. tools/install.py won't install it, FWIW.

mscdex · 2016-05-21T13:15:03Z

I somehow missed the PR link the first time. I rescind my -1 vote.

eugeneo · 2016-05-23T17:09:56Z

I updated the commit message & moved the array. Please take another look.

bnoordhuis · 2016-05-23T18:45:08Z

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

indutny · 2016-05-23T19:07:42Z

If it is not public - should we add something like this to the header?

#ifdef NODE_WANT_INTERNALS
... all code goes here ...
#endif

Sorry, but not LGTM to me yet.

Node already has support for base64 encoding and decoding that is not visible outside the string_bytes.cc file. Our work on providing a support for the Chrome inspector protocol (#6792) requires base64 encoding support. Rather then introducing a second copy of the base64 encoder, we suggest moving this code into a separate header.

eugeneo · 2016-05-23T19:29:28Z

@indutny I've added the condition to a header. Please note that it looks like there are no other headers with such directives - e.g. node-internal.h does not have the condition, it is node.h that conditionally includes node-internal.h.

indutny · 2016-05-23T19:40:34Z

@eugeneo thank you! I know that it is not present anywhere else, but if we don't want this header to be used in general user addons, we have to protect it somehow!

indutny · 2016-05-23T19:40:49Z

LGTM

ofrobots · 2016-05-23T22:47:31Z

LGTM. New CI: https://ci.nodejs.org/job/node-test-pull-request/2752/

Node already has support for base64 encoding and decoding that is not visible outside the string_bytes.cc file. Our work on providing a support for the Chrome inspector protocol (#6792) requires base64 encoding support. Rather then introducing a second copy of the base64 encoder, we suggest moving this code into a separate header. PR-URL: #6910 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

ofrobots · 2016-05-23T23:02:39Z

Landed as 706e699.

bnoordhuis · 2016-05-24T09:07:12Z

I'm with @eugeneo that adding the NODE_WANT_INTERNALS guard is incongruent when it's the only header doing that.

@indutny If you think it's a good idea (I don't disagree), then you should come up with a PR that applies it to all the header files we feel are for internal use only.

bnoordhuis · 2016-05-24T10:02:10Z

#6948

For consistency with the newly added src/base64.h header, check that NODE_WANT_INTERNALS is defined and set in internal headers. PR-URL: nodejs#6948 Refs: nodejs#6910 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Node already has support for base64 encoding and decoding that is not visible outside the string_bytes.cc file. Our work on providing a support for the Chrome inspector protocol (nodejs#6792) requires base64 encoding support. Rather then introducing a second copy of the base64 encoder, we suggest moving this code into a separate header. PR-URL: nodejs#6910 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

For consistency with the newly added src/base64.h header, check that NODE_WANT_INTERNALS is defined and set in internal headers. PR-URL: nodejs#6948 Refs: nodejs#6910 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Node already has support for base64 encoding and decoding that is not visible outside the string_bytes.cc file. Our work on providing a support for the Chrome inspector protocol (#6792) requires base64 encoding support. Rather then introducing a second copy of the base64 encoder, we suggest moving this code into a separate header. PR-URL: #6910 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

For consistency with the newly added src/base64.h header, check that NODE_WANT_INTERNALS is defined and set in internal headers. PR-URL: #6948 Refs: #6910 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins · 2016-06-03T01:34:45Z

@ofrobots @indutny @bnoordhuis @AndreasMadsen should this be included in #7048?

bnoordhuis · 2016-06-03T09:13:42Z

@thealphanerd No, but it should be included if/when the V8 inspector changes are back-ported.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 21, 2016

bnoordhuis reviewed May 21, 2016
View reviewed changes

ofrobots closed this May 23, 2016

eugeneo deleted the base64 branch May 23, 2016 23:34

bnoordhuis mentioned this pull request May 24, 2016
src: add include guards to internal headers #6948
Merged

MylesBorins mentioned this pull request Jun 2, 2016
Audit commits not found on v4.x-staging #7116
Closed

MylesBorins added the lts-watch-v4.x label Jun 3, 2016

addaleax mentioned this pull request Jun 3, 2016
src: print backtrace on fatal error #6734
Merged

MylesBorins added inspector Issues and PRs related to the V8 inspector protocol dont-land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

Uh oh!

Uh oh!

Conversation

eugeneo commented May 21, 2016

Uh oh!

mscdex commented May 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented May 21, 2016

Uh oh!

ofrobots commented May 21, 2016

Uh oh!

ofrobots commented May 21, 2016

Uh oh!

mscdex commented May 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented May 21, 2016

Uh oh!

indutny commented May 21, 2016

Uh oh!

bnoordhuisMay 21, 2016

Choose a reason for hiding this comment

Uh oh!

eugeneoMay 23, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 21, 2016

Uh oh!

mscdex commented May 21, 2016

Uh oh!

eugeneo commented May 23, 2016

Uh oh!

bnoordhuis commented May 23, 2016

Uh oh!

indutny commented May 23, 2016

Uh oh!

eugeneo commented May 23, 2016

Uh oh!

indutny commented May 23, 2016

Uh oh!

indutny commented May 23, 2016

Uh oh!

ofrobots commented May 23, 2016

Uh oh!

ofrobots commented May 23, 2016

Uh oh!

bnoordhuis commented May 24, 2016

Uh oh!

bnoordhuis commented May 24, 2016

Uh oh!

MylesBorins commented Jun 3, 2016

Uh oh!

bnoordhuis commented Jun 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mscdex commented May 21, 2016•
edited
Loading

mscdex commented May 21, 2016•
edited
Loading