★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24276Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

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

Closed
wants to merge 2 commits into from

Conversation

Reviewers

@bnoordhuis bnoordhuis

@joyeecheung joyeecheung

@refack refack

@jasnell jasnell

@addaleax addaleax

@cjihrig cjihrig

Assignees
No one assigned
Labels
C++ author ready backport-requested-v10.x lib / src
Projects
None yet
Milestone
No milestone
10 participants
@gabrielschulhof @nodejs-github-bot @refack @danbev @codebytere @bnoordhuis @jasnell @addaleax @cjihrig @joyeecheung
@gabrielschulhof
Copy link
Contributor

commented Nov 9, 2018

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
@gabrielschulhof
src: bundle persistent-to-local methods as class 
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.
Loading status checks…
475efbd

@gabrielschulhof gabrielschulhof requested a review from addaleax Nov 9, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels Nov 9, 2018

@gabrielschulhof gabrielschulhof referenced this pull request Nov 9, 2018

Closed

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

35 of 137 tasks complete
@joyeecheung

joyeecheung approved these changes Nov 9, 2018
View changes

Copy link
Member

left a comment

LGTM with one suggestion, although I wonder if we can just make a Trait for node::Persistent..
src/util.h Outdated
inline v8::Local<TypeName> WeakPersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);
class PersistentToLocal {

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Nov 9, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 9, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 9, 2018 •
edited

Member

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

This comment has been minimized.

Sign in to view
Copy link
@joyeecheung

joyeecheung Nov 9, 2018

Member

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?

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 9, 2018

Member

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)

This comment has been minimized.

Sign in to view
Copy link
@bnoordhuis

bnoordhuis Nov 9, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@gabrielschulhof

gabrielschulhof Nov 9, 2018 •
edited

Author Contributor

@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

refack reviewed Nov 9, 2018
View changes

Copy link
Member

left a comment

Have questions
Show resolved Hide resolved src/util-inl.h Outdated
src/util.h Outdated
inline v8::Local<TypeName> WeakPersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);
class PersistentToLocal {

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 9, 2018

Member

Why a class and not a namespace? they are all static templates?
src/util.h Outdated
inline v8::Local<TypeName> WeakPersistentToLocal(
v8::Isolate* isolate,
const Persistent<TypeName>& persistent);
class PersistentToLocal {

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Nov 9, 2018 •
edited

Member

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

cjihrig approved these changes Nov 9, 2018
View changes

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Nov 9, 2018

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

addaleax approved these changes Nov 9, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 9, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Nov 10, 2018 •
edited

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

jasnell approved these changes Nov 10, 2018
View changes

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Nov 11, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Nov 11, 2018 •
edited

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

refack approved these changes Nov 12, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 12, 2018

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

@refack refack added the author ready label Nov 12, 2018

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 13, 2018

Landed in 0603c0a.

@danbev danbev closed this Nov 13, 2018

danbev added a commit that referenced this pull request Nov 13, 2018

@gabrielschulhof @danbev
src: bundle persistent-to-local methods as class 
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 <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Loading status checks…
0603c0a

BridgeAR added a commit that referenced this pull request Nov 14, 2018

@gabrielschulhof @BridgeAR
src: bundle persistent-to-local methods as class 
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 <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Unverified
The committer email address is not verified.
Learn about signing commits
7601cdf

@BridgeAR BridgeAR referenced this pull request Nov 14, 2018

Merged

Release proposal: v11.2 #24350

kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018

@gabrielschulhof @kiyomizumia
src: bundle persistent-to-local methods as class 
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 <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
776bf41

This was referenced Nov 15, 2018

Closed
Closed
Closed
Closed

This was referenced Nov 16, 2018

Closed
Closed
@codebytere

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 12, 2019 •
edited

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.