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

gabrielschulhof · 2018-11-09T16:05:23Z

Create a class PersistentToLocal which contains three methods,
Strong, Weak, and Default:

Strong returns a Local from a strong persistent reference,
Weak returns a Local from a weak persistent reference, and
Default decides based on IsWeak() which of the above two to call.

These replace node::StrongPersistentToLocal(),
node::WeakPersistentToLocal(), and node::PersistentToLocal(),
respectively.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Create a class `PersistentToLocal` which contains three methods, `Strong`, `Weak`, and `Default`: * `Strong` returns a `Local` from a strong persistent reference, * `Weak` returns a `Local` from a weak persistent reference, and * `Default` decides based on `IsWeak()` which of the above two to call. These replace `node::StrongPersistentToLocal()`, `node::WeakPersistentToLocal()`, and `node::PersistentToLocal()`, respectively.

nodejs-github-bot · 2018-11-09T16:05:25Z

@gabrielschulhof build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1557/pipeline

joyeecheung

LGTM with one suggestion, although I wonder if we can just make a Trait for node::Persistent..

joyeecheung · 2018-11-09T16:32:02Z

src/util.h

-inline v8::Local<TypeName> WeakPersistentToLocal(
-    v8::Isolate* isolate,
-    const Persistent<TypeName>& persistent);
+class PersistentToLocal {


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

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

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

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?

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)

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.

@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.

refack

Have questions

src/util-inl.h

refack · 2018-11-09T17:06:29Z

src/util.h

-inline v8::Local<TypeName> WeakPersistentToLocal(
-    v8::Isolate* isolate,
-    const Persistent<TypeName>& persistent);
+class PersistentToLocal {


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

refack · 2018-11-09T17:11:02Z

src/util.h

-inline v8::Local<TypeName> WeakPersistentToLocal(
-    v8::Isolate* isolate,
-    const Persistent<TypeName>& persistent);
+class PersistentToLocal {


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

gabrielschulhof · 2018-11-09T21:08:52Z

@refack I made a class, because a class can be aliased with the using keyword. It's true that we might want to move this into node_persistent.h.

refack · 2018-11-09T23:39:06Z

because a class can be aliased with the using keyword

If I understand what you mean, I think that you could alias a namespaced template function just as you can a class:

namespace PersistentToLocal {
  // 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 v8::Persistent<TypeName>& persistent);
};

using Pdo = PersistentToLocal::Default<v8::Object>;

gabrielschulhof · 2018-11-10T04:02:36Z

I moved the definitions into node_persistent.h and placed them inline.

gabrielschulhof · 2018-11-11T04:31:28Z

@refack

using Pdo = PersistentToLocal::Default<v8::Object>;

This works for aliasing the concrete function PersistentToLocal::Default<v8::Object> but it doesn't work if you want to alias the entire templated function.

gabrielschulhof · 2018-11-11T04:32:53Z

@joyeecheung I have heeded your advice and moved the code to node_persistent.h.

refack · 2018-11-12T01:53:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18544/

danbev · 2018-11-13T04:28:59Z

Landed in 0603c0a.

Create a class `PersistentToLocal` which contains three methods, `Strong`, `Weak`, and `Default`: * `Strong` returns a `Local` from a strong persistent reference, * `Weak` returns a `Local` from a weak persistent reference, and * `Default` decides based on `IsWeak()` which of the above two to call. These replace `node::StrongPersistentToLocal()`, `node::WeakPersistentToLocal()`, and `node::PersistentToLocal()`, respectively. PR-URL: #24276 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Create a class `PersistentToLocal` which contains three methods, `Strong`, `Weak`, and `Default`: * `Strong` returns a `Local` from a strong persistent reference, * `Weak` returns a `Local` from a weak persistent reference, and * `Default` decides based on `IsWeak()` which of the above two to call. These replace `node::StrongPersistentToLocal()`, `node::WeakPersistentToLocal()`, and `node::PersistentToLocal()`, respectively. PR-URL: nodejs#24276 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

codebytere · 2019-01-12T19:21:46Z

@gabrielschulhof do you think this should be backported to 10.x?

gabrielschulhof requested a review from addaleax November 9, 2018 16:05

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 9, 2018

gabrielschulhof mentioned this pull request Nov 9, 2018

src: factor out Node.js-agnostic N-APIs #23786

Closed

joyeecheung approved these changes Nov 9, 2018

View reviewed changes

refack reviewed Nov 9, 2018

View reviewed changes

cjihrig approved these changes Nov 9, 2018

View reviewed changes

addaleax approved these changes Nov 9, 2018

View reviewed changes

move definitions to node_persistent.h

0021294

jasnell approved these changes Nov 10, 2018

View reviewed changes

refack approved these changes Nov 12, 2018

View reviewed changes

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 12, 2018

danbev closed this Nov 13, 2018

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 16, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#235

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#236

Closed

codebytere added the backport-requested-v10.x label Jan 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrielschulhof commented Nov 9, 2018

nodejs-github-bot commented Nov 9, 2018

joyeecheung left a comment

joyeecheung Nov 9, 2018

refack Nov 9, 2018

refack Nov 9, 2018 •

edited

Loading

joyeecheung Nov 9, 2018

refack Nov 9, 2018

bnoordhuis Nov 9, 2018

gabrielschulhof Nov 9, 2018 •

edited

Loading

refack left a comment

refack Nov 9, 2018

refack Nov 9, 2018 •

edited

Loading

gabrielschulhof commented Nov 9, 2018

refack commented Nov 9, 2018 •

edited

Loading

gabrielschulhof commented Nov 10, 2018 •

edited

Loading

gabrielschulhof commented Nov 11, 2018

gabrielschulhof commented Nov 11, 2018 •

edited

Loading

refack commented Nov 12, 2018

danbev commented Nov 13, 2018

codebytere commented Jan 12, 2019 •

edited

Loading

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

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

Conversation

gabrielschulhof commented Nov 9, 2018

Checklist

nodejs-github-bot commented Nov 9, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Nov 9, 2018

Choose a reason for hiding this comment

refack Nov 9, 2018

Choose a reason for hiding this comment

refack Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Nov 9, 2018

Choose a reason for hiding this comment

refack Nov 9, 2018

Choose a reason for hiding this comment

bnoordhuis Nov 9, 2018

Choose a reason for hiding this comment

gabrielschulhof Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack Nov 9, 2018

Choose a reason for hiding this comment

refack Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

gabrielschulhof commented Nov 9, 2018

refack commented Nov 9, 2018 • edited Loading

gabrielschulhof commented Nov 10, 2018 • edited Loading

gabrielschulhof commented Nov 11, 2018

gabrielschulhof commented Nov 11, 2018 • edited Loading

refack commented Nov 12, 2018

danbev commented Nov 13, 2018

codebytere commented Jan 12, 2019 • edited Loading

refack Nov 9, 2018 •

edited

Loading

gabrielschulhof Nov 9, 2018 •

edited

Loading

refack Nov 9, 2018 •

edited

Loading

refack commented Nov 9, 2018 •

edited

Loading

gabrielschulhof commented Nov 10, 2018 •

edited

Loading

gabrielschulhof commented Nov 11, 2018 •

edited

Loading

codebytere commented Jan 12, 2019 •

edited

Loading