Fix HandleScope docs #287
Closed
Fix HandleScope docs #287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out `HandleScope::New` doesn't actually exist, so I'm not sure exactly how that should happen. Side note: Maybe it should, because I'm getting the error "Entering the V8 API without proper locking in place" when I just construct `HandleScope` so I think I'm missing something important that could be hidden using a factory function, but I don't know because I don't know why I'm running into this issue. =)
NickNaso
reviewed
Jun 18, 2018
There was a problem hiding this comment.
Just some suggestions.
@gabrielschulhof @mhdawson @kfarnung Someone of you have some time to review this PR?
doc/handle_scope.md
Outdated
| @@ -15,10 +15,10 @@ the section titled (Object lifetime management)[object_lifetime_management]. | |||
| Creates a new handle scope. | |||
|
|
|||
| ```cpp | |||
| HandleScope HandleScope::New(Napi:Env env); | |||
| HandleScope::HandleScope(Napi:Env env); | |||
doc/handle_scope.md
Outdated
| @@ -28,11 +28,11 @@ Returns a new HandleScope | |||
| Creates a new handle scope. | |||
|
|
|||
| ```cpp | |||
| HandleScope HandleScope::New(napi_env env, napi_handle_scope scope); | |||
| HandleScope::HandleScope(Napi::Env env, Napi::HandleScope scope); | |||
There was a problem hiding this comment.
HandleScope(Napi::Env env, Napi::HandleScope scope);
mhdawson
reviewed
Jun 19, 2018
doc/handle_scope.md
Outdated
| @@ -15,10 +15,10 @@ the section titled (Object lifetime management)[object_lifetime_management]. | |||
| Creates a new handle scope. | |||
There was a problem hiding this comment.
I think we should say "Creates a new handle scope on the stack"
mhdawson
reviewed
Jun 19, 2018
doc/handle_scope.md
Outdated
| @@ -28,11 +28,11 @@ Returns a new HandleScope | |||
| Creates a new handle scope. | |||
|
@rivertam thanks for catching that. You are correct that New does not exist and HandleScopes should be declared on the stack as opposed to allocated. For example: |
|
@rivertam made a few additional suggestions, if you can update PR to include those I'll go ahead and land. |
|
@rivertam do you have time to update this PR based on the comments? |
|
No I'm sorry. I've been working like crazy. Might be able to get to it next week sometime, but it's probably faster and more effective if you just take it from here. |
mhdawson
pushed a commit
that referenced
this pull request
Jun 29, 2018
It turns out `HandleScope::New` doesn't actually exist, fix up doc. PR-URL: #287 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
|
Landed as 86be13a |
kevindavies8
added a commit
to kevindavies8/node-addon-api-Develop
that referenced
this pull request
Aug 24, 2022
It turns out `HandleScope::New` doesn't actually exist, fix up doc. PR-URL: nodejs/node-addon-api#287 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Marlyfleitas
added a commit
to Marlyfleitas/node-api-addon-Development
that referenced
this pull request
Aug 26, 2022
It turns out `HandleScope::New` doesn't actually exist, fix up doc. PR-URL: nodejs/node-addon-api#287 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
wroy7860
added a commit
to wroy7860/addon-api-benchmark-node
that referenced
this pull request
Sep 19, 2022
It turns out `HandleScope::New` doesn't actually exist, fix up doc. PR-URL: nodejs/node-addon-api#287 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
johnfrench3
pushed a commit
to johnfrench3/node-addon-api-git
that referenced
this pull request
Aug 11, 2023
It turns out `HandleScope::New` doesn't actually exist, fix up doc. PR-URL: nodejs/node-addon-api#287 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out
HandleScope::Newdoesn't actually exist, so I'm not sure exactly how that happened.Side note: Maybe it should, because I'm getting the error "Entering the V8 API without proper locking in place" when I just construct
HandleScopeso I think I'm missing something important that could be hidden using a factory function, but I don't know because I don't know why I'm running into this issue. =)