Skip to content

Fix HandleScope docs#287

Closed
rivertam wants to merge 3 commits intonodejs:masterfrom
rivertam:patch-3
Closed

Fix HandleScope docs#287
rivertam wants to merge 3 commits intonodejs:masterfrom
rivertam:patch-3

Conversation

@rivertam
Copy link
Copy Markdown
Contributor

@rivertam rivertam commented Jun 18, 2018 •
edited
Loading

It turns out HandleScope::New doesn'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 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. =)

@rivertam
Fix HandleScope docs
b23ead6 
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
NickNaso reviewed Jun 18, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Just some suggestions.
@gabrielschulhof @mhdawson @kfarnung Someone of you have some time to review this PR?

Comment thread doc/handle_scope.md Outdated

```cpp
HandleScope HandleScope::New(Napi:Env env);
HandleScope::HandleScope(Napi:Env env);
Copy link
Copy Markdown
Member

@NickNaso NickNaso Jun 18, 2018

Choose a reason for hiding this comment

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

HandleScope(Napi::Env env);

Comment thread doc/handle_scope.md Outdated

```cpp
HandleScope HandleScope::New(napi_env env, napi_handle_scope scope);
HandleScope::HandleScope(Napi::Env env, Napi::HandleScope scope);
Copy link
Copy Markdown
Member

@NickNaso NickNaso Jun 18, 2018

Choose a reason for hiding this comment

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

HandleScope(Napi::Env env, Napi::HandleScope scope);

mhdawson
mhdawson reviewed Jun 19, 2018
View reviewed changes
Comment thread doc/handle_scope.md Outdated
@@ -15,10 +15,10 @@ the section titled (Object lifetime management)[object_lifetime_management].
Creates a new handle scope.
Copy link
Copy Markdown
Member

@mhdawson mhdawson Jun 19, 2018

Choose a reason for hiding this comment

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

I think we should say "Creates a new handle scope on the stack"

mhdawson
mhdawson reviewed Jun 19, 2018
View reviewed changes
Comment thread doc/handle_scope.md Outdated
@@ -28,11 +28,11 @@ Returns a new HandleScope
Creates a new handle scope.
Copy link
Copy Markdown
Member

@mhdawson mhdawson Jun 19, 2018

Choose a reason for hiding this comment

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

Same comment as above

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jun 19, 2018

@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:

Value createScope(const CallbackInfo& info) {
  {
    HandleScope scope(info.Env());
    String::New(info.Env(), "inner-scope");
  }
  return String::New(info.Env(), "scope");
}

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jun 19, 2018

@rivertam made a few additional suggestions, if you can update PR to include those I'll go ahead and land.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jun 27, 2018

@rivertam do you have time to update this PR based on the comments?

@rivertam
Copy link
Copy Markdown
Contributor Author

rivertam commented Jun 27, 2018

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
mhdawson approved these changes Jun 29, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Jun 29, 2018
@rivertam @mhdawson
doc: Fix HandleScope docs
86be13a 
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>
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Jun 29, 2018

Landed as 86be13a

@mhdawson mhdawson closed this Jun 29, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
doc: Fix HandleScope docs
e40cabd 
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
@Marlyfleitas
doc: Fix HandleScope docs
50e6851 
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
@wroy7860
doc: Fix HandleScope docs
12a3bd0 
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
doc: Fix HandleScope docs
1bf4069 
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

Reviewers

@NickNaso NickNaso NickNaso left review comments

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rivertam @mhdawson @NickNaso