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

src: explain implementation of vm module by fhinkel · Pull Request #16962 · nodejs/node · GitHub

/ node Public

src: explain implementation of vm module - #16962

Closed
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:nov/explain-interceptors
Closed

src: explain implementation of vm module#16962
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:nov/explain-interceptors

Conversation

fhinkel commented Nov 12, 2017

Copy link
Copy Markdown
Contributor

The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person (and future me) working on this.

Checklist
  • make lint passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src - but only adding a comment

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 Nov 12, 2017
Comment thread src/node_contextify.cc Outdated

Copy link
Copy Markdown
Contributor

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

exectues -> executes

Comment thread src/node_contextify.cc Outdated

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

archieved -> achieved

fhinkel force-pushed the nov/explain-interceptors branch from 09a86fc to 43a1bb2 Compare November 12, 2017 14:19
The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.
fhinkel force-pushed the nov/explain-interceptors branch from 43a1bb2 to 4982514 Compare November 12, 2017 14:24

jasnell 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

LGTM, this shouldn't need to wait 48 hours to land.

jasnell commented Nov 12, 2017

Copy link
Copy Markdown
Member

Since this is only adding a comment, just going to lint: https://ci.nodejs.org/job/node-test-linter/13499/

jasnell pushed a commit that referenced this pull request Nov 12, 2017
The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.

PR-URL: #16962
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell commented Nov 12, 2017

Copy link
Copy Markdown
Member

Landed in f0c674d

jasnell closed this Nov 12, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
The vm module uses interceptors on the object template. This is not
straight forward and a comment in the source will help the next
person working on this.

PR-URL: #16962
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas mentioned this pull request Nov 13, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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
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.

8 participants


Back | FazBrowse Home | New Git URL