FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: remove unused env->vm_parsing_context_symbol by maclover7 · Pull Request #22034 · nodejs/node · GitHub

/ node Public

src: remove unused env->vm_parsing_context_symbol - #22034

Closed
maclover7 wants to merge 1 commit into
nodejs:masterfrom
maclover7:jm-rm-parsingcontext
ConversationCommits (1)ChecksFiles changed
Closed

src: remove unused env->vm_parsing_context_symbol#22034
maclover7 wants to merge 1 commit into
nodejs:masterfrom
maclover7:jm-rm-parsingcontext

Conversation

maclover7 commented Jul 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jul 31, 2018

Copy link
Copy Markdown
Collaborator

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/384/pipeline

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
TimothyGu suggested changes Jul 31, 2018
View reviewed changes

TimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 31, 2018

Copy link
Copy Markdown
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 commented Jul 31, 2018

Copy link
Copy Markdown
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 commented Jul 31, 2018

Copy link
Copy Markdown
Member

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

maclover7 commented Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

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

TimothyGu commented Aug 2, 2018

Copy link
Copy Markdown
Member

Looks good, thanks!

src: remove unused env->vm_parsing_context_symbol
677cd3b 
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 commented Aug 3, 2018

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16166/

maclover7 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 4, 2018

maclover7 commented Aug 4, 2018

Copy link
Copy Markdown
Contributor Author

Landed in bd2ee60, thank you for the reviews!

maclover7 closed this Aug 4, 2018
maclover7 deleted the jm-rm-parsingcontext branch August 4, 2018 21:08
maclover7 added a commit that referenced this pull request Aug 4, 2018
src: remove unused env->vm_parsing_context_symbol
bd2ee60 
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 commented Aug 5, 2018

Copy link
Copy Markdown
Member

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

TimothyGu commented Aug 5, 2018

Copy link
Copy Markdown
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 added backport-requested-v10.x and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Aug 6, 2018

targos commented Aug 6, 2018

Copy link
Copy Markdown
Member

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
src: remove unused env->vm_parsing_context_symbol
9d80199 
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>
rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
aliceUnhinged613 mentioned this pull request May 16, 2022
Open
This was referenced May 16, 2022
Open
Open
kissofire mentioned this pull request May 16, 2022
Open
wedataintelligence mentioned this pull request May 16, 2022
Open
ron-a20 mentioned this pull request May 16, 2022
Open
molllyn1 mentioned this pull request May 17, 2022
Open
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
src: remove unused env->vm_parsing_context_symbol
c09872b 
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: nodejs/node#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
+3 more reviewers
TimothyGu TimothyGu approved these changes
targos targos approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL