Skip to content
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 %TypedArray% abstract constructor to primordials #36016

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Nov 7, 2020

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 nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 7, 2020
Copy link
Contributor

@aduh95 aduh95 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:

lib/internal/per_context/primordials.js Show resolved Hide resolved
lib/internal/per_context/primordials.js Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr 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-Boss ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 06d4cde to 4c11653 Compare November 7, 2020 09:19
@ExE-Boss ExE-Boss requested a review from aduh95 November 7, 2020 09:21
@ExE-Boss ExE-Boss requested a review from aduh95 November 7, 2020 09:34
@aduh95 aduh95 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-actions github-actions bot 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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@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,

Comment on lines 120 to 122
const original = global[name];
primordials[name] = original;
copyPropsRenamed(original, primordials, name);
Copy link
Contributor Author

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

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-Boss ExE-Boss force-pushed the lib/primordials/add-typed-array branch from 15592ff to 6d64af7 Compare November 8, 2020 01:09
@ExE-Boss ExE-Boss requested a review from aduh95 November 8, 2020 01:10
lib/internal/freeze_intrinsics.js Outdated Show resolved Hide resolved
Comment on lines 120 to 122
const original = global[name];
primordials[name] = original;
copyPropsRenamed(original, primordials, name);
Copy link
Contributor

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-Boss 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 ExE-Boss requested a review from aduh95 November 8, 2020 19:27
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@github-actions 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
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
@github-actions github-actions bot 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-actions bot commented Nov 9, 2020

Landed in aa1eb1f...c925e24

@github-actions github-actions bot closed this Nov 9, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
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 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
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 danielleadams mentioned this pull request Nov 9, 2020
targos pushed a commit that referenced this pull request May 16, 2021
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 pushed a commit that referenced this pull request Jun 11, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants