src: avoid prototype access in binding templates#47913
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Collaborator
nodejs-github-bot commented May 8, 2023
Review requested:
|
nodejs-github-bot added c++ Collaborator
nodejs-github-bot commented May 8, 2023
29 tasks
legendecas approved these changes May 9, 2023
25 tasks
jasnell approved these changes May 10, 2023
Collaborator
nodejs-github-bot commented May 10, 2023
24 tasks
joyeecheung added the commit-queue nodejs-github-bot added commit-queue-failed Collaborator
nodejs-github-bot commented May 11, 2023
Commit Queue failed- Loading data for nodejs/node/pull/47913 ✔ Done loading data for nodejs/node/pull/47913 ----------------------------------- PR info ------------------------------------ Title src: avoid prototype access in binding templates (#47913) Author Joyee Cheung (@joyeecheung) Branch joyeecheung:binding -> nodejs:main Labels c++, lib / src, needs-ci Commits 1 - src: avoid prototype access in binding templates Committers 1 - Joyee Cheung PR-URL: https://github.com/nodejs/node/pull/47913 Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47913 Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 08 May 2023 03:16:57 GMT ✔ Approvals: 2 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/47913#pullrequestreview-1418106668 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/47913#pullrequestreview-1419744904 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2023-05-10T11:42:30Z: https://ci.nodejs.org/job/node-test-pull-request/51743/ - Querying data for job/node-test-pull-request/51743/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4948851579 |
Member
addaleax commented May 11, 2023
I think what would be nice for the PR description/commit message is a motivation for why this change is right, not just what it does. |
23 tasks
MemberAuthor
Do you have any suggestions? I am not sure how to formulate the motivation (because falling back to prototype lookup for these is unnecessary and a bit awkward?). It's like when you define a module.exports you don't do this weird dance: and just do |
MemberAuthor
joyeecheung commented May 18, 2023
Ping @addaleax Do you have any specific suggestions about the commit message? Or should I just use that example above to explain it? |
Member
addaleax commented May 19, 2023
@joyeecheung No strong preferences, just wanted to make sure that it’s just a simplification of the code and otherwise doesn’t really change anything |
Member
legendecas commented May 22, 2023
Yeah, I believe this is a simplification of the code. The cpp format linter is complaining. A re-format might be needed. |
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template. Previously the initialization was similar to: ``` function Binding(){} Binding.prototype.property = ...; module.exports = new Binding; ``` Now it's similar to: ``` module.exports ={property: ... }; ```joyeecheungforce-pushed the binding branch 2 times, most recently from 084dfdd to be2d7edCompareCollaborator
nodejs-github-bot commented May 23, 2023
Collaborator
nodejs-github-bot commented May 23, 2023
19 tasks
Collaborator
nodejs-github-bot commented May 24, 2023
joyeecheung added a commit that referenced this pull request May 24, 2023
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template. Previously the initialization was similar to: ``` function Binding(){} Binding.prototype.property = ...; module.exports = new Binding; ``` Now it's similar to: ``` module.exports ={property: ... }; ``` PR-URL: #47913 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>MemberAuthor
joyeecheung commented May 24, 2023
Landed in c0365fd with the commit message fixed. |
This was referenced May 25, 2023
targos pushed a commit that referenced this pull request May 30, 2023
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template. Previously the initialization was similar to: ``` function Binding(){} Binding.prototype.property = ...; module.exports = new Binding; ``` Now it's similar to: ``` module.exports ={property: ... }; ``` PR-URL: #47913 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>22 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template. Previously the initialization was similar to: ``` function Binding(){} Binding.prototype.property = ...; module.exports = new Binding; ``` Now it's similar to: ``` module.exports ={property: ... }; ``` PR-URL: nodejs#47913 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template. Previously the initialization was similar to: ``` function Binding(){} Binding.prototype.property = ...; module.exports = new Binding; ``` Now it's similar to: ``` module.exports ={property: ... }; ``` PR-URL: nodejs#47913 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch makes the binding templates ObjectTemplates, since we don't actually need the constructor function. This also avoids setting the properties on prototype, and instead initializes them directly on the object template.