-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
lib: add uncurried accessor properties to primordials
#36329
lib: add uncurried accessor properties to primordials
#36329
Conversation
I think that all property getters do not have a setter accessible to users. That would mean there is no "security" reason to add them to primordials, it's just more convenient to not duplicate calls to |
Modifying the code to count and log the number of encountered setters shows that there are 22 setters encountered by the |
Interesting, I stand corrected :) Can you also replace this instance in Lines 119 to 121 in 7d45dd9
|
9cc02c7
to
73e07da
Compare
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/738/console (queued, will 404 until it launches) |
It seems to introduce some performance regressions, but it seems to me it's unrelated code 🤔
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@aduh95 There’s apparently a way to only restart the failed CI builds from the https://ci.nodejs.org page (e.g.: https://ci.nodejs.org/job/node-test-pull-request/34677/) without needing to rerun all CI builds: From #35811 (comment):
|
@ExE-Boss Can you rebase on top of c91e608 to fix the CI failure please? |
73e07da
to
02dad91
Compare
@aduh95 Done. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO this change makes the code harder to read. Would you agree to create a function to avoid repeating the code over and over:
function defineGetterAndSetter(desc, dest, prefix, key) {
const { enumerable, get, set } = desc;
Reflect.defineProperty(dest, `${prefix}Get${key}`, {
value: uncurryThis(get),
enumerable
});
if (set !== undefined) {
Reflect.defineProperty(dest, `${prefix}Set${key}`, {
value: uncurryThis(set),
enumerable
});
}
}
fdcfeb0
to
4ec94c4
Compare
Landed in c83e599 |
Closes: nodejs#32127 PR-URL: nodejs#36329 Fixes: nodejs#32127 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Closes: #32127
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes