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/base_object.h | |
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/base_object.h')
-rw-r--r-- | src/base_object.h | 50 |
1 files changed, 38 insertions, 12 deletions
diff --git a/src/base_object.h b/src/base_object.h index 478499bbfeb..7d8281238b1 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -26,6 +26,7 @@ #include "node_persistent.h" #include "v8.h" +#include <type_traits> // std::remove_reference namespace node { @@ -33,8 +34,10 @@ class Environment; class BaseObject { public: + // Associates this object with `handle`. It uses the 0th internal field for + // that, and in particular aborts if there is no such field. inline BaseObject(Environment* env, v8::Local<v8::Object> handle); - virtual ~BaseObject() = default; + virtual inline ~BaseObject(); // Returns the wrapped object. Returns an empty handle when // persistent.IsEmpty() is true. @@ -44,23 +47,30 @@ class BaseObject { inline Environment* env() const; - // The handle_ must have an internal field count > 0, and the first - // index is reserved for a pointer to this class. This is an - // implicit requirement, but Node does not have a case where it's - // required that MakeWeak() be called and the internal field not - // be set. - template <typename Type> - inline void MakeWeak(Type* ptr); + // Get a BaseObject* pointer, or subclass pointer, for the JS object that + // was also passed to the `BaseObject()` constructor initially. + // This may return `nullptr` if the C++ object has not been constructed yet, + // e.g. when the JS object used `MakeLazilyInitializedJSTemplate`. + static inline BaseObject* FromJSObject(v8::Local<v8::Object> object); + template <typename T> + static inline T* FromJSObject(v8::Local<v8::Object> object); + // Make the `Persistent` a weak reference and, `delete` this object once + // the JS object has been garbage collected. + inline void MakeWeak(); + + // Undo `MakeWeak()`, i.e. turn this into a strong reference. inline void ClearWeak(); + // Utility to create a FunctionTemplate with one internal field (used for + // the `BaseObject*` pointer) and a constructor that initializes that field + // to `nullptr`. + static inline v8::Local<v8::FunctionTemplate> MakeLazilyInitializedJSTemplate( + Environment* env); + private: BaseObject(); - template <typename Type> - static inline void WeakCallback( - const v8::WeakCallbackInfo<Type>& data); - // persistent_handle_ needs to be at a fixed offset from the start of the // class because it is used by src/node_postmortem_metadata.cc to calculate // offsets and generate debug symbols for BaseObject, which assumes that the @@ -71,6 +81,22 @@ class BaseObject { Environment* env_; }; + +// Global alias for FromJSObject() to avoid churn. +template <typename T> +inline T* Unwrap(v8::Local<v8::Object> obj) { + return BaseObject::FromJSObject<T>(obj); +} + + +#define ASSIGN_OR_RETURN_UNWRAP(ptr, obj, ...) \ + do { \ + *ptr = static_cast<typename std::remove_reference<decltype(*ptr)>::type>( \ + BaseObject::FromJSObject(obj)); \ + if (*ptr == nullptr) \ + return __VA_ARGS__; \ + } while (0) + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS |