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: remove unused env->vm_parsing_context_symbol #22034

Closed
wants to merge 1 commit into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Jul 31, 2018

Stopped being used via 77b52fd, was
originally added in d932e80.

For the one remaining usecase inside of lib/vm.js, define a Symbol at
the top of the file.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Jul 31, 2018
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

This is still very much used in https://github.com/nodejs/node/blob/master/lib/vm.js. The way the symbol was used in that file made the tests magically pass (as the string key 'undefined' is used in place).

In that particular file the symbol could just be changed to a regular local one declared with Symbol().

@addaleax
Copy link
Member

@TimothyGu But is the property read anywhere? At a glance, it looks like this would be safe to remove from vm.js as well…?

@maclover7
Copy link
Contributor Author

Ah, sorry, missed the lib/vm.js usecase (got when looking over 77b52fd and read that filename as lib/vm.js by accident)

I agree with @addaleax though that the use in lib/vm.js could be removed -- doing ggrep -r kParsingContext test/ comes up empty, and whatever ggrep -r "new vm.Script(" test/ and ggrep -r "new Script(" . returns doesn't seem to interact with the symbol at all.

@targos
Copy link
Member

targos commented Jul 31, 2018

+1 for removal and using a Symbol() declared in vm.js

@maclover7
Copy link
Contributor Author

@TimothyGu I updated to use a symbol declared inside vm.js -- does this seem okay to you?

@TimothyGu
Copy link
Member

Looks good, thanks!

Stopped being used via 77b52fd, was
originally added in d932e80.

For the one remaining usecase inside of `lib/vm.js`, define a Symbol at
the top of the file.
@maclover7 maclover7 force-pushed the jm-rm-parsingcontext branch from 4641899 to 677cd3b Compare August 3, 2018 04:39
@maclover7
Copy link
Contributor Author

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 4, 2018
@maclover7
Copy link
Contributor Author

Landed in bd2ee60, thank you for the reviews!

@maclover7 maclover7 closed this Aug 4, 2018
@maclover7 maclover7 deleted the jm-rm-parsingcontext branch August 4, 2018 21:08
maclover7 added a commit that referenced this pull request Aug 4, 2018
Stopped being used via 77b52fd, was
originally added in d932e80.

For the one remaining usecase inside of `lib/vm.js`, define a Symbol at
the top of the file.

PR-URL: #22034
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member

addaleax commented Aug 5, 2018

@maclover7 @TimothyGu Uh … this seems to have gotten lost in the discussion, but still, does anything speak against removing this key completely?

@TimothyGu
Copy link
Member

@addaleax The symbol is used to transmit the parsing context information from vm.runInContext and vm.runInNewContext to the Script constructor, without creating a new public option as the Script constructor is public. I'm fine with exposing it as a public option but it met resistance in #14888 (#14888 (comment)).

@targos targos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 6, 2018
@targos
Copy link
Member

targos commented Aug 6, 2018

Depends on #21668 to land on v10.x-staging. More specifically this commit: 104953c

targos pushed a commit that referenced this pull request Aug 11, 2018
Stopped being used via 77b52fd, was
originally added in d932e80.

For the one remaining usecase inside of `lib/vm.js`, define a Symbol at
the top of the file.

PR-URL: #22034
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants