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

commented Dec 31, 2018

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
joyeecheung added 2 commits Dec 31, 2018
buffer: move initialization of buffer prototype into node.js
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.
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 creaton of the `FastBuffer` class into
`internal/buffer.js` and expose it directly instead of
writing it onto that module later.
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

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

class FastBuffer extends Uint8Array {}

function addBufferPrototypeMethods(proto) {
proto.readUIntLE = readUIntLE;

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 2, 2019

Author Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jan 10, 2019

Member

I am fine either way.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

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

@addaleax
Copy link
Member

left a comment

Code LGTM

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@jasnell
jasnell approved these changes Jan 9, 2019
joyeecheung added a commit that referenced this pull request Jan 10, 2019
buffer: move initialization of buffer prototype into node.js
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit that referenced this pull request Jan 10, 2019
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 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

Landed in fa5af0d...842a35f

@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

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

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

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

BridgeAR added a commit that referenced this pull request Jan 16, 2019
buffer: move initialization of buffer prototype into node.js
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit that referenced this pull request Jan 16, 2019
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 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit that referenced this pull request Jan 16, 2019
buffer: move initialization of buffer prototype into node.js
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit that referenced this pull request Jan 16, 2019
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 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
buffer: move initialization of buffer prototype into node.js
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
buffer: move initialization of buffer prototype into node.js
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
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 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

@targos targos added this to Backported in v11.x Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.