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.

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

buffer: make Buffer binding and prototype setup more straight-forward #25292

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

buffer: move initialization of buffer prototype into node.js

Instead of exposing setBufferJS in lib/internal/buffer.js after deleting
it from the binding and then do the initialization in
lib/buffer.js, which results in an implicit dependency on
the order in which these modules are loaded.

buffer: move Buffer prototype wiring into internal/buffer.js

Instead of exposing the Buffer prototype methods through an
object in internal/buffer.js and then iterating over it
to put the methods on the prototype, create a function
in internal/buffer.js to do this.

Also moves the creation of the FastBuffer class into
internal/buffer.js and expose it directly instead of
writing it onto that module later.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Instead of exposing it in `lib/internal/buffer.js` after deleting
it from the binding and then do the initialization in
`lib/buffer.js`, which results in an implicit dependency on
the order in which these modules are loaded.
Instead of exposing the Buffer prototype methods through an
object in `internal/buffer.js` and then iterating over it
to put the methods on the prototype, create a function
in `internal/buffer.js` to do this.

Also moves the creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 31, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Ping @nodejs/process @nodejs/buffer can I have some review please?

class FastBuffer extends Uint8Array {}

function addBufferPrototypeMethods(proto) {
proto.readUIntLE = readUIntLE;
Copy link
Member Author

Choose a reason for hiding this comment

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

Another way to do this is to put all these to the prototype of FastBuffer, I am not sure if there are any concerns about edge cases though

Copy link
Member

Choose a reason for hiding this comment

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

I am fine either way.

@joyeecheung
Copy link
Member Author

Ping again..according to the GitHub reviewer recommendations: @BridgeAR @TimothyGu @addaleax

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.

Code LGTM

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2019
@joyeecheung
Copy link
Member Author

joyeecheung added a commit that referenced this pull request Jan 10, 2019
Instead of exposing it in `lib/internal/buffer.js` after deleting
it from the binding and then do the initialization in
`lib/buffer.js`, which results in an implicit dependency on
the order in which these modules are loaded.

PR-URL: #25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this pull request Jan 10, 2019
Instead of exposing the Buffer prototype methods through an
object in `internal/buffer.js` and then iterating over it
to put the methods on the prototype, create a function
in `internal/buffer.js` to do this.

Also moves the creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.

PR-URL: #25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in fa5af0d...842a35f

@addaleax
Copy link
Member

@joyeecheung Can you look into backporting this to v11.x-staging?

@BridgeAR
Copy link
Member

I pushed a backport directly onto v11.x-staging. There was just a minor conflict in lib/internal/bootstrap/node.js.

BridgeAR pushed a commit that referenced this pull request Jan 16, 2019
Instead of exposing it in `lib/internal/buffer.js` after deleting
it from the binding and then do the initialization in
`lib/buffer.js`, which results in an implicit dependency on
the order in which these modules are loaded.

PR-URL: #25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 16, 2019
Instead of exposing the Buffer prototype methods through an
object in `internal/buffer.js` and then iterating over it
to put the methods on the prototype, create a function
in `internal/buffer.js` to do this.

Also moves the creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.

PR-URL: #25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Instead of exposing it in `lib/internal/buffer.js` after deleting
it from the binding and then do the initialization in
`lib/buffer.js`, which results in an implicit dependency on
the order in which these modules are loaded.

PR-URL: nodejs#25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Instead of exposing the Buffer prototype methods through an
object in `internal/buffer.js` and then iterating over it
to put the methods on the prototype, create a function
in `internal/buffer.js` to do this.

Also moves the creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.

PR-URL: nodejs#25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
Instead of exposing it in `lib/internal/buffer.js` after deleting
it from the binding and then do the initialization in
`lib/buffer.js`, which results in an implicit dependency on
the order in which these modules are loaded.

PR-URL: nodejs#25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
Instead of exposing the Buffer prototype methods through an
object in `internal/buffer.js` and then iterating over it
to put the methods on the prototype, create a function
in `internal/buffer.js` to do this.

Also moves the creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.

PR-URL: nodejs#25292
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants