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: use predefined AliasedBuffer types in the code base #27334

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Instead of allowing the callers to instantiate the template
with any numeric types (such as aliasing a Uint8Array to double[]),
predefine types that make sense and use those instead.

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

Instead of allowing the callers to instantiate the template
with any numeric types (such as aliasing a Uint8Array to double[]),
predefine types that make sense and use those instead.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 21, 2019
@nodejs-github-bot
Copy link
Collaborator

@@ -251,6 +249,12 @@ class AliasedBuffer {
NativeT* buffer_;
v8::Global<V8T> js_array_;
};

typedef AliasedBufferBase<int32_t, v8::Int32Array> AliasedInt32Array;
Copy link
Member Author

Choose a reason for hiding this comment

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

Another reason for doing this is that it would be easier to implement something like AliasedInt32Array::ForEach() if we add a static list in the case like what we do for the BaseObjects - then we'll be able to iterate over all known aliased buffers in the heap snapshot generator, as well as do bookkeeping for snapshot [de]serialization (we'll record the indexes of the TypedArrays and ArrayBuffers in the snapshot during serialization, and deserialize them into C++ later to keep the states consistent)

@joyeecheung joyeecheung added the review wanted PRs that need reviews. label Apr 22, 2019
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung removed the review wanted PRs that need reviews. label Apr 25, 2019
joyeecheung added a commit that referenced this pull request Apr 26, 2019
Instead of allowing the callers to instantiate the template
with any numeric types (such as aliasing a Uint8Array to double[]),
predefine types that make sense and use those instead.

PR-URL: #27334
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in 0ae46a7

targos pushed a commit that referenced this pull request Apr 27, 2019
Instead of allowing the callers to instantiate the template
with any numeric types (such as aliasing a Uint8Array to double[]),
predefine types that make sense and use those instead.

PR-URL: #27334
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 27, 2019
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. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants