GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||
GeezerVison is showing github.com — http2: refactor to avoid unsafe array iteration by aduh95 · Pull Request #36700 · nodejs/node Conversation
Contributor
Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
jasnell
approved these changes
rchougule
reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
P.S. Just started with open-source reviews and was trying to learn from other PRs.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes,
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
This comment has been minimized.
Trott
approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM if benchmarks are OK
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
This comment has been minimized.
This comment has been minimized.
tniessen
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this also "vulnerable" to prototype pollution "attacks," if we really want to consider that "unsafe?" I mean, one could define a getter on the I'm really not sure if all this complexity and illegibility are worth the minimal benefit.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defining a Object.defineProperty(Array.prototype, '0', {get(){return 9}, set(v){/* no op */}}) ((t, ...args)=>args[0])(1, 2, 3) === 2 // true I don't think we are concerned about code being "malicious" anyway, what we are trying to achieve with the move to primordials is to allow users to monkey-patch the built-in objects (for debugging purposes, for tinkering, etc.) without having errors thrown by Node.js internals.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Might be, I am not sure.
I understand the motivation, but I am not sure it's worth the cost to maintainability, readability etc.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To address the maintainability concern, would you prefer if we added a linter rule to forbid array destructuring assignment?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
aduh95
force-pushed
the
htt2-array-iteration
branch
from
bf0fb18 to
f261d70
Compare
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
|
Replacing Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/889 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: nodejs#36700 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
5cdcc12 to
0fd9bbb
Compare
|
Landed in 0fd9bbb |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: #36700 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.