refactor(NLVObject): use smart pointers for tracking object lifetime#76
refactor(NLVObject): use smart pointers for tracking object lifetime#76
Conversation
|
|
||
| void ClearChildren() { | ||
| for (auto& ptr: children_) { | ||
| if (auto object = ptr.lock()) { |
There was a problem hiding this comment.
@mbroadst this is the reward, ability to see if those weak pointers are stil valid.
|
yeah definitely now that we've committed to c++11, smart pointers are of course the way to go :) |
| HandleType handle_; | ||
| NLVObjectBasePtr* parentReference_; | ||
|
|
||
| std::unique_ptr<HandleValue, decltype(&CleanupHandler::cleanup)> handle_; |
|
@Rush I left a few comments. I really like the direction this is taking, but I wonder if we can't split the single commit into at least two:
It's a bit hard to review this mono-commit version because of all the Thoughts? |
|
Sure, I'll split it later this evening probably. Thanks for leaving the comments. |
Rush
force-pushed
the
refactor/smart-pointers
branch
from
9794cfe to
a8051f8
Compare
src/nlv_object.h
Outdated
| HandleType handle() const { | ||
| return handle_; | ||
| } | ||
| const HandleType handle() const { |
There was a problem hiding this comment.
as a purely informational refactor, do you think it would be a good idea to rename this to virHandle() so we avoid mental collisions with v8's handle() ?
There was a problem hiding this comment.
otherwise I have no issues with this commit, I'll just cherry-pick it right in
There was a problem hiding this comment.
Good idea. Do you have the time to rename it yourself?
mbroadst
force-pushed
the
refactor/smart-pointers
branch
from
a8051f8 to
925e37e
Compare
2 participants
@mbroadst I had a lazy evening and a crazy idea to use weak pointers instead of
NLVObjectPtr. Note:enable_shared_from_thisis probably not necessary but I wanted you to see this branch earlier rather than later to get some comments.