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

smalloc: extend user API #905

Closed
wants to merge 1 commit into from
Closed

Conversation

trevnorris
Copy link
Contributor

node::Environment isn't accessible to user APIs, so extend smalloc to
also accept v8::Isolate.

Fixes: 75adde0 "src: remove node_isolate from source"

R=@bnoordhuis
R=@indutny



void AllocDispose(Isolate* isolate, Handle<Object> obj) {
AllocDispose(isolate, obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be AllocDispose(Environment::GetCurrent(isolate), obj)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe. i'll add that to the test.

@indutny
Copy link
Member

indutny commented Feb 20, 2015

LGTM, if it works



// Internal use
void Alloc(Environment* env,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move it to smalloc-internal.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so then I'd recommend doing that in a different PR, and doing with all other headers that do the same (e.g. node_buffer.h).

node::Environment isn't accessible to user APIs, so extend smalloc to
also accept v8::Isolate.

Fixes: 75adde0 "src: remove `node_isolate` from source"
@trevnorris
Copy link
Contributor Author

@vkurchatkin addressed comments. thanks.

trevnorris added a commit that referenced this pull request Feb 20, 2015
node::Environment isn't accessible to user APIs, so extend smalloc to
also accept v8::Isolate.

Fixes: 75adde0 "src: remove `node_isolate` from source"
PR-URL: #905
Reviewed-by: Fedor Indutny <fedor@indutny.com>
@trevnorris
Copy link
Contributor Author

Landed in 26ebe98.

@trevnorris trevnorris closed this Feb 20, 2015
@trevnorris trevnorris deleted the extend-smalloc branch February 20, 2015 18:09
trevnorris added a commit to nodejs/node-v0.x-archive that referenced this pull request Feb 20, 2015
node::Environment isn't accessible to user APIs, so extend smalloc to
also accept v8::Isolate.

Fixes: 75adde0 "src: remove `node_isolate` from source"
PR-URL: nodejs/node#905
Reviewed-by: Fedor Indutny <fedor@indutny.com>
@rvagg
Copy link
Member

rvagg commented Feb 22, 2015

should this have the semver-minor tag? is it a feature add?

@trevnorris
Copy link
Contributor Author

@rvagg No. It's a fix to an unusable API. Check the "Fixes" in the commit message for the commit that broke it. This happened when Environment was added as an API argument, thus making it impossible to use from a native module.

This was referenced Feb 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants