From bd201102862a194f3f5ce669e0a3c8143eafc900 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 1 May 2018 18:34:52 +0200 Subject: src: refactor `BaseObject` internal field management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Reviewed-By: Ben Noordhuis Reviewed-By: Daniel Bevenius --- src/node_serdes.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/node_serdes.cc') diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 6ace942c29..520b350199 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -86,7 +86,7 @@ class DeserializerContext : public BaseObject, SerializerContext::SerializerContext(Environment* env, Local wrap) : BaseObject(env, wrap), serializer_(env->isolate(), this) { - MakeWeak(this); + MakeWeak(); } void SerializerContext::ThrowDataCloneError(Local message) { @@ -274,7 +274,7 @@ DeserializerContext::DeserializerContext(Environment* env, deserializer_(env->isolate(), data_, length_, this) { object()->Set(env->context(), env->buffer_string(), buffer).FromJust(); - MakeWeak(this); + MakeWeak(); } MaybeLocal DeserializerContext::ReadHostObject(Isolate* isolate) { -- cgit v1.2.3