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: use Isolate::TryGetCurrent where appropriate #39954

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

In two places, we call Isolate::GetCurrent() even though that is
technically invalid usage of the function.
Now that V8 exposes Isolate::TryGetCurrent(), we can do this
in a proper way.

In two places, we call `Isolate::GetCurrent()` even though that is
technically invalid usage of the function.
Now that V8 exposes `Isolate::TryGetCurrent()`, we can do this
in a proper way.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 31, 2021
@danbev
Copy link
Contributor

danbev commented Aug 31, 2021

We should be able to revert 1fc4d43 after this commit. I can open a pull request after this one has landed, or if you like you can include the revert in this pr.

@addaleax
Copy link
Member Author

@danbev Sure, done!

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Still LGTM

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2021
@@ -91,7 +91,7 @@ inline T MultiplyWithOverflowCheck(T a, T b);

namespace per_process {
// Tells whether the per-process V8::Initialize() is called and
// if it is safe to call v8::Isolate::GetCurrent().
// if it is safe to call v8::Isolate::TryGetCurrent().
extern bool v8_initialized;
Copy link
Member

@legendecas legendecas Sep 5, 2021

Choose a reason for hiding this comment

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

Do we still need this as it claiming that it tells "if it is safe to call v8::Isolate::GetCurrent"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah … I don’t know why, but v8::Isolate::TryGetCurrent() is not something you can call without V8 being initialized first.

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2021

Landed in 9f6fed7...9f7412a

@github-actions github-actions bot closed this Sep 8, 2021
nodejs-github-bot pushed a commit that referenced this pull request Sep 8, 2021
In two places, we call `Isolate::GetCurrent()` even though that is
technically invalid usage of the function.
Now that V8 exposes `Isolate::TryGetCurrent()`, we can do this
in a proper way.

PR-URL: #39954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Sep 8, 2021
This reverts commit 1fc4d43.

PR-URL: #39954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
In two places, we call `Isolate::GetCurrent()` even though that is
technically invalid usage of the function.
Now that V8 exposes `Isolate::TryGetCurrent()`, we can do this
in a proper way.

PR-URL: #39954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
This reverts commit 1fc4d43.

PR-URL: #39954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants