Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

jsg::Lock cleanup #551

Merged
merged 2 commits into from
Apr 20, 2023
Merged

jsg::Lock cleanup #551

merged 2 commits into from
Apr 20, 2023

Conversation

jclee
Copy link
Contributor

@jclee jclee commented Apr 19, 2023

Addresses a couple cleanup items on PR #547:

  • Renames a few cases of jsgLock variables to js to match convention.
  • Adds a jsg::Lock::v8Context() convenience function and updates code to use it.

@jclee
Copy link
Contributor Author

jclee commented Apr 19, 2023

A few things I wasn't quite sure about:

  • having a member function v8Context() is convenient when used analogously to v8Isolate, but it differs from other workerd code in that it's a method that doesn't start with a verb. I think the convention would be getContext() or getV8Context(), but that's a bit more verbose. We also have a fair number of preexisting getContext() methods; not sure if having another might lead to confusion.

  • I notice Worker::Lock has an analogous function -- another getContext() -- but it doesn't do quite the same thing, so I didn't try to rewrite code to use it, and likewise didn't rewrite things like lock.getIsolate()->GetCurrentContext().

  • I generally left alone anything that wasn't obviously already using jsg::Lock, so there are a bunch of other places in the code still calling GetCurrentContext(). Mostly, they seem to be places where a v8::Isolate* parameter is being passed instead of a lock reference.

  • Some places in the code seem to be jsg::Lock derivatives (e.g. JsgWorkerdIsolate::Lock), although I noticed they were using lock instead of js. Wasn't sure if this was intentional, so also I left them alone.

@jclee jclee requested a review from jasnell April 19, 2023 18:36
@jclee jclee merged commit cf63ae0 into main Apr 20, 2023
@jclee jclee deleted the jlee/jsglock-cleanup branch May 31, 2024 07:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants