Skip to content

lib: add %TypedArray% abstract constructor to primordials#36016

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.

Sign up for GitHub

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

Jump to bottom
Closed
ExE-Boss wants to merge 1 commit into from
Closed

lib: add %TypedArray% abstract constructor to primordials #36016

ExE-Boss wants to merge 1 commit into from

Conversation

@ExE-Boss
Copy link
Contributor

@ExE-BossExE-Boss commented Nov 7, 2020•
edited
Loading

Refs:#35448
Refs:#36003
Refs:https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Supersedes and closes#32127

review?(@aduh95, @targos)

@nodejs-github-botnodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 7, 2020
@ExE-BossExE-Boss mentioned this pull request Nov 7, 2020
Closed
2 tasks
aduh95
aduh95 approved these changes Nov 7, 2020
View reviewed changes
Copy link
Contributor

@aduh95aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this! I like it way more than my hack in the other PR. Two comments:

benjamingr
benjamingr approved these changes Nov 7, 2020
View reviewed changes
Copy link
Member

@benjamingrbenjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Antoine's comments addressed :]

@ExE-BossExE-Bossforce-pushed the lib/primordials/add-typed-array branch from 06d4cde to 4c11653CompareNovember 7, 2020 09:19
@ExE-BossExE-Boss requested a review from aduh95November 7, 2020 09:21
aduh95
aduh95 reviewed Nov 7, 2020
View reviewed changes
@ExE-BossExE-Boss requested a review from aduh95November 7, 2020 09:34
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 7, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Nov 7, 2020

nit: you can also replace two instances in freeze_intrinsics.js:

--- a/lib/internal/freeze_intrinsics.js+++ b/lib/internal/freeze_intrinsics.js@@ -65,6 +65,7 @@ const{SymbolIterator, SyntaxError, TypeError, + TypedArrayPrototype, Uint16Array, Uint32Array, Uint8Array, @@ -105,7 +106,7 @@ module.exports = function(){// AsyncGeneratorFunction ObjectGetPrototypeOf(async function* (){}), // TypedArray - ObjectGetPrototypeOf(Uint8Array),+ TypedArrayPrototype, // 19 Fundamental Objects Object.prototype, // 19.1 @@ -189,7 +190,7 @@ module.exports = function(){// AsyncGeneratorFunction ObjectGetPrototypeOf(async function* (){}), // TypedArray - ObjectGetPrototypeOf(Uint8Array),+ TypedArrayPrototype, // 18 The Global Object eval,

@ExE-BossExE-Bossforce-pushed the lib/primordials/add-typed-array branch from 4c11653 to 15592ffCompareNovember 8, 2020 01:04
@ExE-BossExE-Boss mentioned this pull request Nov 8, 2020
Closed
3 tasks
ExE-Boss
ExE-Boss commented Nov 8, 2020
View reviewed changes
lib/internal/per_context/primordials.js Outdated
Comment on lines 120 to 122
Copy link
ContributorAuthor

@ExE-BossExE-BossNov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed because of #35448 (comment).

Copy link
Contributor

@aduh95aduh95Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open a separate PR for this?

freeze_intrinsics.js is executed before any user code, I don't think it's worth adding it to the primordials object: there's no security reason to do so, IMHO we should fix frozen_intrinsics.js code instead.

@ExE-BossExE-Bossforce-pushed the lib/primordials/add-typed-array branch from 15592ff to 6d64af7CompareNovember 8, 2020 01:09
@ExE-BossExE-Boss requested a review from aduh95November 8, 2020 01:10
aduh95
aduh95 reviewed Nov 8, 2020
View reviewed changes
lib/internal/per_context/primordials.js Outdated
Comment on lines 120 to 122
Copy link
Contributor

@aduh95aduh95Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you open a separate PR for this?

freeze_intrinsics.js is executed before any user code, I don't think it's worth adding it to the primordials object: there's no security reason to do so, IMHO we should fix frozen_intrinsics.js code instead.

Leko
Leko approved these changes Nov 8, 2020
View reviewed changes
@aduh95aduh95 mentioned this pull request Nov 8, 2020
Closed
2 tasks
@ExE-BossExE-Bossforce-pushed the lib/primordials/add-typed-array branch 2 times, most recently from aca2ee7 to 8fee9b7CompareNovember 8, 2020 19:27
@ExE-BossExE-Boss requested a review from aduh95November 8, 2020 19:27
@aduh95aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 8, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/34218/

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
@github-actionsgithub-actionsbot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
@github-actions
Copy link
Contributor

github-actionsbot commented Nov 9, 2020

Landed in aa1eb1f...c925e24

nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
@ExE-Boss@nodejs-github-bot
lib: add %TypedArray% abstract constructor to primordials
c925e24
Refs: #35448 Refs: #36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <[email protected]> PR-URL: #36016 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@ExE-BossExE-Boss deleted the lib/primordials/add-typed-array branch November 9, 2020 10:00
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@ExE-Boss@danielleadams
lib: add %TypedArray% abstract constructor to primordials
af2a560
Refs: #35448 Refs: #36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <[email protected]> PR-URL: #36016 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Nov 9, 2020
Merged
targos pushed a commit that referenced this pull request May 16, 2021
@ExE-Boss@targos
lib: add %TypedArray% abstract constructor to primordials
2fd37db
Refs: #35448 Refs: #36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <[email protected]> PR-URL: #36016 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
@targostargos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@ExE-Boss@targos
lib: add %TypedArray% abstract constructor to primordials
24d4d63
Refs: #35448 Refs: #36003 Refs: https://tc39.es/ecma262/#sec-%typedarray%-intrinsic-object Co-authored-by: Antoine du Hamel <[email protected]> PR-URL: #36016 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes

@aduh95aduh95aduh95 approved these changes

+1 more reviewer

@LekoLekoLeko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.utilIssues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ExE-Boss@nodejs-github-bot@aduh95@benjamingr@Leko