-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
bootstrap_node: remove side effects on RegExp #19138
Conversation
To clarify why I took this approach instead of running the regexes in a new context: Contexts are really memory-expensive, and I'd rather not have to wait for the GC to activate for something we will use only once. It would also complicate the effort to create V8 snapshots for Node.js core (see #17058). |
Micro-nit: I'd prefer a more descriptive name for the test and/or a bit more comment explaining what the test is testing for. I'm fine with this landing without these concerns addressed. Just my own opinion/suggestion. Take it or leave it. |
this seems like a band-aid, can't we just preprocess the gypi file at build time? |
Yes, but I personally did not want to get into the build system 😄 Feel free to propose a better approach with a new pull request. |
// convert the Python syntax to proper JSON | ||
// We cannot use a regex due to side effects on static properties of RegExp | ||
// (e.g., RegExp.$_). | ||
let out = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe a more descriptive name than "out"?
the test will fail on windows for the reasons seen in #19140 (comment) |
Closing, as #19140 has been landed. |
something to note, 19140 did not fix the fact that the regexp object is still poisoned on windows |
Fixes: #18930
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
bootstrap_node