diff options
author | Anna Henningsen <anna@addaleax.net> | 2018-05-01 19:34:52 +0300 |
---|---|---|
committer | Anna Henningsen <anna@addaleax.net> | 2018-05-04 01:57:39 +0300 |
commit | bd201102862a194f3f5ce669e0a3c8143eafc900 (patch) | |
tree | 22c86fcd4bff3716704c981f14fa80c3a6dd0d6b /src/cares_wrap.cc | |
parent | a9b0d8235d5ae11a5ae0748f05c4d290a848f1b9 (diff) |
src: refactor `BaseObject` internal field management
- Instead of storing a pointer whose type refers to the specific
subclass of `BaseObject`, just store a `BaseObject*` directly.
This means in particular that one can cast to classes along
the way of the inheritance chain without issues, and that
`BaseObject*` no longer needs to be the first superclass
in the case of multiple inheritance.
In particular, this renders hack-y solutions to this problem (like
ddc19be6de1ba263d9c175b2760696e7b9918b25) obsolete and addresses
a `TODO` comment of mine.
- Move wrapping/unwrapping methods to the `BaseObject` class.
We use these almost exclusively for `BaseObject`s, and I hope
that this gives a better idea of how (and for what) these are used
in our code.
- Perform initialization/deinitialization of the internal field
in the `BaseObject*` constructor/destructor. This makes the code
a bit more obviously correct, avoids explicit calls for this
in subclass constructors, and in particular allows us to avoid
crash situations when we previously called `ClearWrap()`
during GC.
This also means that we enforce that the object passed to the
`BaseObject` constructor needs to have an internal field.
This is the only reason for the test change.
- Change the signature of `MakeWeak()` to not require a pointer
argument. Previously, this would always have been the same
as `this`, and no other value made sense. Also, the parameter
was something that I personally found somewhat confusing
when becoming familiar with Node’s code.
- Add a `TODO` comment that motivates switching to real inheritance
for the JS types we expose from the native side. This patch
brings us a lot closer to being able to do that.
- Some less significant drive-by cleanup.
Since we *effectively* already store the `BaseObject*` pointer
anyway since ddc19be6de1ba263d9c175b2760696e7b9918b25, I do not
think that this is going to have any impact on diagnostic tooling.
Fixes: https://github.com/nodejs/node/issues/18897
PR-URL: https://github.com/nodejs/node/pull/20455
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Diffstat (limited to 'src/cares_wrap.cc')
-rw-r--r-- | src/cares_wrap.cc | 31 |
1 files changed, 4 insertions, 27 deletions
diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 0cc7ed1464f..4df47d75d43 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -187,7 +187,7 @@ ChannelWrap::ChannelWrap(Environment* env, is_servers_default_(true), library_inited_(false), active_query_count_(0) { - MakeWeak<ChannelWrap>(this); + MakeWeak(); Setup(); } @@ -205,7 +205,6 @@ class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> { GetAddrInfoReqWrap(Environment* env, Local<Object> req_wrap_obj, bool verbatim); - ~GetAddrInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } bool verbatim() const { return verbatim_; } @@ -219,18 +218,12 @@ GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env, bool verbatim) : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETADDRINFOREQWRAP) , verbatim_(verbatim) { - Wrap(req_wrap_obj, this); -} - -GetAddrInfoReqWrap::~GetAddrInfoReqWrap() { - ClearWrap(object()); } class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> { public: GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj); - ~GetNameInfoReqWrap(); size_t self_size() const override { return sizeof(*this); } }; @@ -238,11 +231,6 @@ class GetNameInfoReqWrap : public ReqWrap<uv_getnameinfo_t> { GetNameInfoReqWrap::GetNameInfoReqWrap(Environment* env, Local<Object> req_wrap_obj) : ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETNAMEINFOREQWRAP) { - Wrap(req_wrap_obj, this); -} - -GetNameInfoReqWrap::~GetNameInfoReqWrap() { - ClearWrap(object()); } @@ -587,8 +575,6 @@ class QueryWrap : public AsyncWrap { QueryWrap(ChannelWrap* channel, Local<Object> req_wrap_obj) : AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP), channel_(channel) { - Wrap(req_wrap_obj, this); - // Make sure the channel object stays alive during the query lifetime. req_wrap_obj->Set(env()->context(), env()->channel_string(), @@ -597,7 +583,6 @@ class QueryWrap : public AsyncWrap { ~QueryWrap() override { CHECK_EQ(false, persistent().IsEmpty()); - ClearWrap(object()); } // Subclasses should implement the appropriate Send method. @@ -2143,14 +2128,8 @@ void Initialize(Local<Object> target, target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "AI_V4MAPPED"), Integer::New(env->isolate(), AI_V4MAPPED)); - auto is_construct_call_callback = - [](const FunctionCallbackInfo<Value>& args) { - CHECK(args.IsConstructCall()); - ClearWrap(args.This()); - }; Local<FunctionTemplate> aiw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - aiw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, aiw); Local<String> addrInfoWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "GetAddrInfoReqWrap"); @@ -2158,8 +2137,7 @@ void Initialize(Local<Object> target, target->Set(addrInfoWrapString, aiw->GetFunction()); Local<FunctionTemplate> niw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - niw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, niw); Local<String> nameInfoWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "GetNameInfoReqWrap"); @@ -2167,8 +2145,7 @@ void Initialize(Local<Object> target, target->Set(nameInfoWrapString, niw->GetFunction()); Local<FunctionTemplate> qrw = - FunctionTemplate::New(env->isolate(), is_construct_call_callback); - qrw->InstanceTemplate()->SetInternalFieldCount(1); + BaseObject::MakeLazilyInitializedJSTemplate(env); AsyncWrap::AddWrapMethods(env, qrw); Local<String> queryWrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "QueryReqWrap"); |