Skip to content

tools: check for std::vector<v8::Local> in lint #58497

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Aditi-1400
Copy link
Contributor

Adds a lint rule to disallow the usage of std::vector<v8::Local<T>>.
Follow up from the clean up of std::vector<v8::Local<T>> we did before.
From pull request: #57578:

According to V8's public API documentation, local handles (i.e., objects of type v8::Local) "should never be allocated on the heap". This replaces the usage of heap-allocated data structures containing instances of v8::Local, specifically the std::vector<v8::Localv8::String> with recently introduced v8::LocalVector.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 28, 2025
@Aditi-1400
Copy link
Contributor Author

Oops, looks like I missed a few instances of std::vector<v8::Local<T>>. I'll take care of those before this pull request is merged

Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

lgtm

@anonrig
Copy link
Member

anonrig commented May 30, 2025

I don't think we need this. We can just enable V8_ENABLE_LOCAL_OFF_STACK_CHECK check on v8 that enforces vector not to be used.

@joyeecheung
Copy link
Member

We can just enable V8_ENABLE_LOCAL_OFF_STACK_CHECK check on v8 that enforces vector not to be used.

I think we can have both, in case it ends up in a path that we don't actually build/test in the CI.

@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 9, 2025
@joyeecheung
Copy link
Member

src/quic/streams.h:335:  Do not use std::vector<v8::Local<T>>. Use v8::LocalVector<T> instead.  [runtime/local_vector] [5]
test/addons/make-callback/binding.cc:14:  Do not use std::vector<v8::Local<T>>. Use v8::LocalVector<T> instead.  [runtime/local_vector] [5]
test/addons/openssl-providers/binding.cc:29:  Do not use std::vector<v8::Local<T>>. Use v8::LocalVector<T> instead.  [runtime/local_vector] [5]

This is getting being caught in the CI of this PR BTW (kinda proves my point about "what if the path is not built in the CI"..)

@joyeecheung
Copy link
Member

I think the GitHub actions require manual rebase. @Aditi-1400 can you rebase again?

@Aditi-1400 Aditi-1400 force-pushed the disallow-local-vector branch from 1e9a155 to 2632670 Compare June 11, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants