★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25292Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
+120 −95

Conversation

Reviewers

@BridgeAR BridgeAR

@jasnell jasnell

@addaleax addaleax

Assignees
No one assigned
Labels
C++ author ready backported-to-v11.x lib / src
Projects
  
Backported in v11.x
Milestone
No milestone
5 participants
@joyeecheung @nodejs-github-bot @addaleax @BridgeAR @jasnell
@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 some commits Dec 31, 2018

@joyeecheung
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.
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
fa6b492
@joyeecheung
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.
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
8d5c144
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels Dec 31, 2018

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 31, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/19894/
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 2, 2019

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

joyeecheung reviewed Jan 2, 2019
View changes

lib/internal/buffer.js
class FastBuffer extends Uint8Array {}

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

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link
@BridgeAR

BridgeAR Jan 10, 2019

Member

I am fine either way.
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 4, 2019

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

addaleax approved these changes Jan 4, 2019
View changes

Copy link
Member

left a comment

Code LGTM

@joyeecheung joyeecheung added the author ready label Jan 9, 2019

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 9, 2019

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/20023/
@jasnell

jasnell approved these changes Jan 9, 2019
View changes

@BridgeAR

BridgeAR approved these changes Jan 10, 2019
View changes

joyeecheung added a commit that referenced this pull request Jan 10, 2019

@joyeecheung
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>
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
97f59b9

joyeecheung added a commit that referenced this pull request Jan 10, 2019

@joyeecheung
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>
Verified
This commit was signed with a verified signature.
@joyeecheung joyeecheung Joyee Cheung
GPG key ID: 92B78A53C8303B8D Learn about signing commits
Loading status checks…
842a35f
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 10, 2019

Landed in fa5af0d...842a35f

@joyeecheung joyeecheung closed this Jan 10, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 15, 2019

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

@addaleax addaleax added the backport-requested-v11.x label Jan 15, 2019

@BridgeAR

This comment has been minimized.

Sign in to view
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

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
2ae4a76

BridgeAR added a commit that referenced this pull request Jan 16, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
a0088ff

@BridgeAR BridgeAR added backported-to-v11.x and removed backport-requested-v11.x labels Jan 16, 2019

BridgeAR added a commit that referenced this pull request Jan 16, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
255f017

BridgeAR added a commit that referenced this pull request Jan 16, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
eacc151

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
edd19ce

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
6cf4922

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
042d20a

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

@joyeecheung @BridgeAR
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>
Unverified
The committer email address is not verified.
Learn about signing commits
8e84ccb

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

@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
You can’t perform that action at this time.