Skip to content

src: bundle persistent-to-local methods as class #24276

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
gabrielschulhof wants to merge 2 commits into from
Closed

src: bundle persistent-to-local methods as class #24276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
475efbd
src: bundle persistent-to-local methods as class
Nov 7, 2018
0021294
move definitions to node_persistent.h
Nov 10, 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: 2 additions & 1 deletion src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,8 @@ void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {
HandleScope scope(info.GetIsolate());

std::unique_ptr<DestroyParam> p{info.GetParameter()};
Local<Object> prop_bag = PersistentToLocal(info.GetIsolate(), p->propBag);
Local<Object> prop_bag = PersistentToLocal::Default(info.GetIsolate(),
p->propBag);
Local<Value> val;

if (!prop_bag->Get(p->env->context(), p->env->destroyed_string())
Expand Down
2 changes: 1 addition & 1 deletion src/base_object-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ Persistent<v8::Object>& BaseObject::persistent() {


v8::Local<v8::Object> BaseObject::object() const {
return PersistentToLocal(env_->isolate(), persistent_handle_);
return PersistentToLocal::Default(env_->isolate(), persistent_handle_);
}

v8::Local<v8::Object> BaseObject::object(v8::Isolate* isolate) const {
Expand Down
2 changes: 1 addition & 1 deletion src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,7 @@ void Environment::ForEachBaseObject(T&& iterator) {

#define V(PropertyName, TypeName) \
inline v8::Local<TypeName> Environment::PropertyName() const { \
return StrongPersistentToLocal(PropertyName ## _); \
return PersistentToLocal::Strong(PropertyName ## _); \
} \
inline void Environment::set_ ## PropertyName(v8::Local<TypeName> value) { \
PropertyName ## _.Reset(isolate(), value); \
Expand Down
2 changes: 1 addition & 1 deletion src/heap_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class JSGraphJSNode : public EmbedderGraph::Node {
const char* Name() override { return "<JS Node>"; }
size_t SizeInBytes() override { return 0; }
bool IsEmbedderNode() override { return false; }
Local<Value> JSValue() { return StrongPersistentToLocal(persistent_); }
Local<Value> JSValue() { return PersistentToLocal::Strong(persistent_); }

int IdentityHash() {
Local<Value> v = JSValue();
Expand Down
2 changes: 1 addition & 1 deletion src/memory_tracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class NodeBIO;
* // a BaseObject or an AsyncWrap class
* bool IsRootNode() const override { return !wrapped_.IsWeak(); }
* v8::Local<v8::Object> WrappedObject() const override {
* return node::PersistentToLocal(wrapped_);
* return node::PersistentToLocal::Default(wrapped_);
* }
* private:
* AnotherRetainerClass another_retainer_;
Expand Down
2 changes: 1 addition & 1 deletion src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ struct napi_env__ {
node::Persistent<v8::Context> context_persistent;

inline v8::Local<v8::Context> context() const {
return StrongPersistentToLocal(context_persistent);
return node::PersistentToLocal::Strong(context_persistent);
}

inline node::Environment* node_env() const {
Expand Down
4 changes: 2 additions & 2 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ void ContextifyScript::CreateCachedData(
ContextifyScript* wrapped_script;
ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder());
Local<UnboundScript> unbound_script =
PersistentToLocal(env->isolate(), wrapped_script->script_);
PersistentToLocal::Default(env->isolate(), wrapped_script->script_);
std::unique_ptr<ScriptCompiler::CachedData> cached_data(
ScriptCompiler::CreateCodeCache(unbound_script));
if (!cached_data) {
Expand Down Expand Up @@ -863,7 +863,7 @@ bool ContextifyScript::EvalMachine(Environment* env,
ContextifyScript* wrapped_script;
ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false);
Local<UnboundScript> unbound_script =
PersistentToLocal(env->isolate(), wrapped_script->script_);
PersistentToLocal::Default(env->isolate(), wrapped_script->script_);
Local<Script> script = unbound_script->BindToCurrentContext();

MaybeLocal<Value> result;
Expand Down
2 changes: 1 addition & 1 deletion src/node_contextify.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class ContextifyContext {
}

inline v8::Local<v8::Context> context() const {
return PersistentToLocal(env()->isolate(), context_);
return PersistentToLocal::Default(env()->isolate(), context_);
}

inline v8::Local<v8::Object> global_proxy() const {
Expand Down
3 changes: 2 additions & 1 deletion src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2342,7 +2342,8 @@ int SSLWrap<Base>::TLSExtStatusCallback(SSL* s, void* arg) {
if (w->ocsp_response_.IsEmpty())
return SSL_TLSEXT_ERR_NOACK;

Local<Object> obj = PersistentToLocal(env->isolate(), w->ocsp_response_);
Local<Object> obj = PersistentToLocal::Default(env->isolate(),
w->ocsp_response_);
char* resp = Buffer::Data(obj);
size_t len = Buffer::Length(obj);

Expand Down
8 changes: 0 additions & 8 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,14 +182,6 @@ extern std::shared_ptr<PerProcessOptions> per_process_opts;
// Forward declaration
class Environment;

// If persistent.IsWeak() == false, then do not call persistent.Reset()
// while the returned Local<T> is still in scope, it will destroy the
// reference to the object.
template <class TypeName>
inline v8::Local<TypeName> PersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);

// Convert a struct sockaddr to a { address: '1.2.3.4', port: 1234 } JS object.
// Sets address and port properties on the info object and returns it.
// If |info| is omitted, a new object is returned.
Expand Down
4 changes: 2 additions & 2 deletions src/node_zlib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,8 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
UpdateWriteResult();

// call the write() cb
Local<Function> cb = PersistentToLocal(env()->isolate(),
write_js_callback_);
Local<Function> cb = PersistentToLocal::Default(env()->isolate(),
write_js_callback_);
MakeCallback(cb, 0, nullptr);

if (pending_close_)
Expand Down
10 changes: 5 additions & 5 deletions src/util-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,25 +166,25 @@ constexpr ContainerOfHelper<Inner, Outer> ContainerOf(Inner Outer::*field,
}

template <class TypeName>
inline v8::Local<TypeName> PersistentToLocal(
inline v8::Local<TypeName> PersistentToLocal::Default(
Comment thread
refack marked this conversation as resolved.
Outdated
v8::Isolate* isolate,
const Persistent<TypeName>& persistent) {
if (persistent.IsWeak()) {
return WeakPersistentToLocal(isolate, persistent);
return PersistentToLocal::Weak(isolate, persistent);
} else {
return StrongPersistentToLocal(persistent);
return PersistentToLocal::Strong(persistent);
}
}

template <class TypeName>
inline v8::Local<TypeName> StrongPersistentToLocal(
inline v8::Local<TypeName> PersistentToLocal::Strong(
const Persistent<TypeName>& persistent) {
return *reinterpret_cast<v8::Local<TypeName>*>(
const_cast<Persistent<TypeName>*>(&persistent));
}

template <class TypeName>
inline v8::Local<TypeName> WeakPersistentToLocal(
inline v8::Local<TypeName> PersistentToLocal::Weak(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent) {
return v8::Local<TypeName>::New(isolate, persistent);
Expand Down
45 changes: 24 additions & 21 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,27 +201,30 @@ template <typename Inner, typename Outer>
constexpr ContainerOfHelper<Inner, Outer> ContainerOf(Inner Outer::*field,
Inner* pointer);

// If persistent.IsWeak() == false, then do not call persistent.Reset()
// while the returned Local<T> is still in scope, it will destroy the
// reference to the object.
template <class TypeName>
inline v8::Local<TypeName> PersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);

// Unchecked conversion from a non-weak Persistent<T> to Local<T>,
// use with care!
//
// Do not call persistent.Reset() while the returned Local<T> is still in
// scope, it will destroy the reference to the object.
template <class TypeName>
inline v8::Local<TypeName> StrongPersistentToLocal(
const Persistent<TypeName>& persistent);

template <class TypeName>
inline v8::Local<TypeName> WeakPersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);
class PersistentToLocal {
Copy link
Copy Markdown
Member

@joyeecheung joyeecheung Nov 9, 2018

Choose a reason for hiding this comment

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

Shouldn't this be put into node_persistent.h? Since these are all node::Persistent

Copy link
Copy Markdown
Contributor

@refack refack Nov 9, 2018

Choose a reason for hiding this comment

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

Why a class and not a namespace? they are all static templates?

Copy link
Copy Markdown
Contributor

@refack refack Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

P.S. why not move the implementations here? They are all single lines (some could even be constexpr on C++17 compilers)

You can do feature detection with either __cpp_constexpr < 201304 or __cpp_constexpr < 201603 (I think 201603 is needed for v8:Local<> return values)

// Wordaround a GCC4.9 bug that C++14 N3652 was not implemented
// Refs: https://www.gnu.org/software/gcc/projects/cxx-status.html#cxx14
// Refs: https://isocpp.org/files/papers/N3652.html
#if __cpp_constexpr < 201304
#  define constexpr 
#endif

.
.
.

#undef constexpr

Copy link
Copy Markdown
Member

@joyeecheung joyeecheung Nov 9, 2018

Choose a reason for hiding this comment

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

Doesn't that bring more complexity than necessary? (the constexpr approach) How is it different from doing process.versions.v8 detection in JS and use harmony features?

Copy link
Copy Markdown
Contributor

@refack refack Nov 9, 2018

Choose a reason for hiding this comment

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

Doesn't that bring more complexity than necessary? (the constexpr approach)

A little bit, yes. So up to author. BTW I think there's a CONSTEXPR macro defined by V8 based of this condition, so we could reuse.

How is it different from doing process.versions.v8 detection in JS and use harmony features?

It's compile time only, 0 run time cost, only benefits. (only cost is the above mentioned code complexity)

Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Nov 9, 2018

Choose a reason for hiding this comment

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

Shouldn't this be put into node_persistent.h? Since these are all node::Persistent

You mean as node::Persistent methods? I'd say that's the best/most logical place for it.

Copy link
Copy Markdown
Contributor Author

@gabrielschulhof gabrielschulhof Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@bnoordhuis I tried adding them as node::Persistent methods, but node::Persistent is merely an alias to v8::Persistent, so I can't really extend it, AFAIK.

public:
// If persistent.IsWeak() == false, then do not call persistent.Reset()
// while the returned Local<T> is still in scope, it will destroy the
// reference to the object.
template <class TypeName>
static inline v8::Local<TypeName> Default(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);

// Unchecked conversion from a non-weak Persistent<T> to Local<T>,
// use with care!
//
// Do not call persistent.Reset() while the returned Local<T> is still in
// scope, it will destroy the reference to the object.
template <class TypeName>
static inline v8::Local<TypeName> Strong(
const Persistent<TypeName>& persistent);

template <class TypeName>
static inline v8::Local<TypeName> Weak(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);
};

// Convenience wrapper around v8::String::NewFromOneByte().
inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
Expand Down