Skip to content

src: use smart pointer in AsyncWrap::WeakCallback #19168

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

Closed
danbev wants to merge 2 commits into from
Closed

src: use smart pointer in AsyncWrap::WeakCallback #19168

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7c25ee
src: use smart pointer in AsyncWrap::WeakCallback
danbev Mar 6, 2018
36f0e37
squash: add comment about scope of unique_ptr
danbev Mar 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,14 +415,13 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {
HandleScope scope(info.GetIsolate());

Environment* env = Environment::GetCurrent(info.GetIsolate());
DestroyParam* p = info.GetParameter();
std::unique_ptr<DestroyParam> p{info.GetParameter()};

@addaleax addaleax Mar 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Can you add a comment here that says that assigning the pointer will destroy the object at the end of the function?

I like smart pointers, but I think this is one of the cases where the ownership flow becomes a bit less obvious. I find it hard to put a finger on what exactly it is, but I think it’s mostly that we have a new call that creates the object and works with it as a raw pointer on the one end, and use a smart pointer on the other side (i.e. there’s no matching delete call)…

@danbev danbev Mar 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problems, I'll add a comment. Thanks

Local<Object> prop_bag = PersistentToLocal(info.GetIsolate(), p->propBag);

Local<Value> val = prop_bag->Get(env->destroyed_string());
if (val->IsFalse()) {
AsyncWrap::EmitDestroy(env, p->asyncId);
}
delete p;
}


Expand Down