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.

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

src: null env_ field from constructor #2913

Closed
wants to merge 1 commit into from
Closed

src: null env_ field from constructor #2913

wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"

R=@indutny
R=@Fishrock123

@trevnorris
src: null env_ field from constructor
c573c14 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 16, 2015
@trevnorris
Copy link
Contributor Author

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

@indutny
Copy link
Member

indutny commented Sep 16, 2015

LGTM, but please wait for CI ;) (see @nodejs/punished )

@rvagg
Copy link
Member

rvagg commented Sep 16, 2015

shaming via github team membership 💥

trevnorris added a commit that referenced this pull request Sep 16, 2015
@trevnorris
src: null env_ field from constructor
3b78b85 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2913
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris
Copy link
Contributor Author

Thanks. CI looks good. Landed in 3b78b85.

There are failures of AssertionError: 'Invalid array buffer length' == 'Buffer allocation failed - process out of memory'. These are because the error message thrown from the Uint8Array() constructor is different than the message previously thrown if it failed to allocate memory from C++. Unrelated to this PR.

@trevnorris trevnorris closed this Sep 16, 2015
trevnorris added a commit that referenced this pull request Sep 16, 2015
@trevnorris @Fishrock123
src: null env_ field from constructor
8e58434 
The env_ field in ArrayBufferAllocator needs to be null'd out since it
is used during initialization and checked prior to properly being set by
set_env().

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2913
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris trevnorris deleted the fix-my-f-up branch September 16, 2015 20:39
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as 8e58434

Also pretty jazzed to find out the history of @nodejs/punished

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@trevnorris @indutny @rvagg @MylesBorins @mscdex @jasnell