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

src: remove calls to deprecated ArrayBuffer methods #32358

Closed
wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 19, 2020

v8::ArrayBuffer::IsExternal and v8::ArrayBuffer::Externalize are
no longer necessary.

@targos targos requested a review from addaleax March 19, 2020 10:32
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 19, 2020
@targos targos mentioned this pull request Mar 19, 2020
4 tasks
@nodejs-github-bot
Copy link
Collaborator

BridgeAR
BridgeAR previously approved these changes Mar 19, 2020
@targos
Copy link
Member Author

targos commented Mar 19, 2020

One N-API test fails:

assert.js:102
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at /home/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel7-s390x/test/js-native-api/test_typedarray/test.js:81:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel7-s390x/test/js-native-api/test_typedarray/test.js:79:12)
    at Module._compile (internal/modules/cjs/loader.js:1202:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1222:10)
    at Module.load (internal/modules/cjs/loader.js:1051:32)
    at Function.Module._load (internal/modules/cjs/loader.js:947:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: /A detachable arraybuffer was expected/,
  operator: 'throws'
}

@BridgeAR BridgeAR dismissed their stale review March 19, 2020 10:49

Removing my LGTM until the CI is green.

@targos
Copy link
Member Author

targos commented Mar 19, 2020

I'm not sure what we are supposed to do. Call ab->Detach() ?

@gengjiawen
Copy link
Member

@mmarchini has some suggestion in nodejs/node-v8#149

@gengjiawen gengjiawen added the v8 engine Issues and PRs related to the V8 dependency. label Mar 19, 2020
@mmarchini
Copy link
Contributor

The patch below should fix the failing test, but it would be good to get input from someone more familiar with ArrayBuffers.

diff --git a/test/js-native-api/test_typedarray/test.js b/test/js-native-api/test_typedarray/test.js
index 0d9594d929..f3efcc6e23 100644
--- a/test/js-native-api/test_typedarray/test.js
+++ b/test/js-native-api/test_typedarray/test.js
@@ -78,9 +78,7 @@ nonByteArrayTypes.forEach((currentType) => {
 // Test detaching
 arrayTypes.forEach((currentType) => {
   const buffer = Reflect.construct(currentType, [8]);
-  assert.throws(
-    () => test_typedarray.Detach(buffer),
-    /A detachable arraybuffer was expected/);
+  assert.doesNotThrow(() => test_typedarray.Detach(buffer));
 });
 {
   const buffer = test_typedarray.External();

v8::ArrayBuffer::IsExternal and v8::ArrayBuffer::Externalize are
no longer necessary.
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Mar 20, 2020

adapted the test to be more meaningful

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 20, 2020
targos added a commit that referenced this pull request Mar 21, 2020
v8::ArrayBuffer::IsExternal and v8::ArrayBuffer::Externalize are
no longer necessary.

PR-URL: #32358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@targos
Copy link
Member Author

targos commented Mar 21, 2020

Landed in d23eed2

@targos targos closed this Mar 21, 2020
@targos targos deleted the remove-external branch March 21, 2020 11:01
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. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants