FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: add %TypedArray% abstract constructor to primordials by ExE-Boss · Pull Request #36016 · nodejs/node · GitHub

/ node Public

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

Closed
ExE-Boss wants to merge 1 commit into
nodejs:masterfrom
ExE-Boss:lib/primordials/add-typed-array
ConversationCommits (1)ChecksFiles changed
Closed

lib: add %TypedArray% abstract constructor to primordials#36016
ExE-Boss wants to merge 1 commit into
nodejs:masterfrom
ExE-Boss:lib/primordials/add-typed-array

Conversation

ExE-Boss commented Nov 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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-bot added the util Issues and PRs related to the built-in util module. label Nov 7, 2020
ExE-Boss mentioned this pull request Nov 7, 2020
Closed
2 tasks
aduh95 approved these changes Nov 7, 2020
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/internal/per_context/primordials.js Outdated
Comment thread lib/internal/per_context/primordials.js Outdated
benjamingr approved these changes Nov 7, 2020
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With Antoine's comments addressed :]

ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 06d4cde to 4c11653 Compare November 7, 2020 09:19
ExE-Boss requested a review from aduh95 November 7, 2020 09:21
aduh95 reviewed Nov 7, 2020
View reviewed changes
Comment thread lib/internal/per_context/primordials.js Outdated
ExE-Boss requested a review from aduh95 November 7, 2020 09:34
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 7, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2020

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

aduh95 commented Nov 7, 2020

Copy link
Copy Markdown
Contributor

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-Boss force-pushed the lib/primordials/add-typed-array branch from 4c11653 to 15592ff Compare November 8, 2020 01:04
ExE-Boss mentioned this pull request Nov 8, 2020
Closed
3 tasks
ExE-Boss commented Nov 8, 2020
View reviewed changes
Comment thread lib/internal/per_context/primordials.js Outdated
Comment on lines 120 to 122

ExE-Boss Nov 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is needed because of #35448 (comment).

aduh95 Nov 8, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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-Boss force-pushed the lib/primordials/add-typed-array branch from 15592ff to 6d64af7 Compare November 8, 2020 01:09
ExE-Boss requested a review from aduh95 November 8, 2020 01:10
aduh95 reviewed Nov 8, 2020
View reviewed changes
Comment thread lib/internal/freeze_intrinsics.js Outdated
Comment thread lib/internal/per_context/primordials.js Outdated
Comment on lines 120 to 122

aduh95 Nov 8, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

aduh95 mentioned this pull request Nov 8, 2020
Closed
2 tasks
ExE-Boss force-pushed the lib/primordials/add-typed-array branch 2 times, most recently from aca2ee7 to 8fee9b7 Compare November 8, 2020 19:27
ExE-Boss requested a review from aduh95 November 8, 2020 19:27
aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020

nodejs-github-bot commented Nov 8, 2020

Copy link
Copy Markdown
Collaborator

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020

github-actions Bot commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

Landed in aa1eb1f...c925e24

github-actions Bot closed this Nov 9, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
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 <duhamelantoine1995@gmail.com>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
ExE-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
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 <duhamelantoine1995@gmail.com>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
danielleadams mentioned this pull request Nov 9, 2020
Merged
targos pushed a commit that referenced this pull request May 16, 2021
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 <duhamelantoine1995@gmail.com>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
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 <duhamelantoine1995@gmail.com>

PR-URL: #36016
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes
+1 more reviewer
Leko Leko approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. util Issues 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

Back | FazBrowse Home | New Git URL