Skip to content
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,test,tools: forbid usage of v8::Persistent #31018

Closed
wants to merge 4 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Dec 18, 2019

src,test: use v8::Global instead of v8::Persistent

This is in preparation for a lint rule forbidding usage of v8::Persistent.

doc: avoid using v8::Persistent in addon docs

Use v8::Global where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.

tools,src: forbid usage of v8::Persistent

v8::Persistent comes with the surprising catch that it requires manual cleanup. v8::Global doesn’t, making it easier to use, and additionally provides move semantics. New code should always use v8::Global.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@addaleax addaleax mentioned this pull request Dec 18, 2019
4 of 4 tasks complete
addaleax added 4 commits Dec 18, 2019
This is in preparation for a lint rule forbidding usage of
`v8::Persistent`.
`v8::Persistent` comes with the surprising catch that it requires
manual cleanup. `v8::Global` doesn’t, making it easier to use,
and additionally provides move semantics. New code should always
use `v8::Global`.
Use `v8::Global` where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.
@addaleax addaleax force-pushed the addaleax:no-persistent branch from 8eaa539 to 0c0d530 Dec 18, 2019
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Dec 18, 2019

@cjihrig @richardlau Can you take another look? I had to push some changes to the addon docs, as those weren’t linted when running make lint locally.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Dec 18, 2019

Still LGTM

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Dec 18, 2019
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Dec 19, 2019

Any ideas why the CI comment here is marked outdated or who hid it? Afaict, this should be ready with a yellow CI.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 19, 2019

Any ideas why the CI comment here is marked outdated or who hid it? Afaict, this should be ready with a yellow CI.

Sorry, that was me. I misread GitHub and thought that 335d450 was newer than 0c0d530 but having checked again I see I was mistaken 🙇 and have unhidden the comment now.
image

@Qard
Qard approved these changes Dec 22, 2019
Copy link
Member

Qard left a comment

LGTM, with a minor nit, but I don't want it to block landing this--just something to think about.

doc/api/addons.md Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Dec 24, 2019

Landed in db109e8...e23bf8f

@addaleax addaleax closed this Dec 24, 2019
addaleax added a commit that referenced this pull request Dec 24, 2019
This is in preparation for a lint rule forbidding usage of
`v8::Persistent`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
addaleax added a commit that referenced this pull request Dec 24, 2019
Use `v8::Global` where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
addaleax added a commit that referenced this pull request Dec 24, 2019
`v8::Persistent` comes with the surprising catch that it requires
manual cleanup. `v8::Global` doesn’t, making it easier to use,
and additionally provides move semantics. New code should always
use `v8::Global`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@addaleax addaleax deleted the addaleax:no-persistent branch Dec 24, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
This is in preparation for a lint rule forbidding usage of
`v8::Persistent`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
Use `v8::Global` where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
`v8::Persistent` comes with the surprising catch that it requires
manual cleanup. `v8::Global` doesn’t, making it easier to use,
and additionally provides move semantics. New code should always
use `v8::Global`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
This is in preparation for a lint rule forbidding usage of
`v8::Persistent`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
targos added a commit that referenced this pull request Jan 14, 2020
Use `v8::Global` where possible. For examples where it applies,
also clean up the code and make the code multi-threading-ready,
for those where that isn’t easily possible, add a warning about that.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
targos added a commit that referenced this pull request Jan 14, 2020
`v8::Persistent` comes with the surprising catch that it requires
manual cleanup. `v8::Global` doesn’t, making it easier to use,
and additionally provides move semantics. New code should always
use `v8::Global`.

PR-URL: #31018
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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.