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

trace_events: move SetupTraceCategoryState into node_trace_events.cc by joyeecheung · Pull Request #25128 · nodejs/node · GitHub

/ node Public

trace_events: move SetupTraceCategoryState into node_trace_events.cc - #25128

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:move-trace-setup
ConversationCommits (2)ChecksFiles changed
Closed

trace_events: move SetupTraceCategoryState into node_trace_events.cc#25128
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:move-trace-setup

Conversation

joyeecheung commented Dec 19, 2018

Copy link
Copy Markdown
Member

It makes more sense to put it in internalBinding('trace_events')
instead of in the bootstrapper object.

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

trace_events: move SetupTraceCategoryState into node_trace_events.cc
1b44481 
It makes more sense to put it in `internalBinding('trace_events')`
instead of in the bootstrapper object.

nodejs-github-bot commented Dec 19, 2018

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 19, 2018

joyeecheung commented Dec 19, 2018

Copy link
Copy Markdown
Member Author

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

targos reviewed Dec 19, 2018
View reviewed changes
Comment thread lib/internal/bootstrap/node.js Outdated
const { traceCategoryState } = internalBinding('trace_events');
const {
traceCategoryState,
setupTraceCategoryState

targos Dec 19, 2018

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

Could you use another name (avoid that of the current executing function)?

joyeecheung commented Dec 19, 2018

Copy link
Copy Markdown
Member Author

@targos Thanks for catching that, I updated the internal function name to setTraceCategoryStateUpdateHandler since that's what it actually does. PTAL.

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 19, 2018

joyeecheung commented Dec 20, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Dec 21, 2018

Copy link
Copy Markdown
Member Author

Resume: https://ci.nodejs.org/job/node-test-pull-request/19708/

joyeecheung added a commit that referenced this pull request Dec 21, 2018
trace_events: move SetupTraceCategoryState into node_trace_events.cc
70f2328 
It makes more sense to put it in `internalBinding('trace_events')`
instead of in the bootstrapper object.

PR-URL: #25128
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung commented Dec 21, 2018

Copy link
Copy Markdown
Member Author

Landed in 70f2328, thanks!

MylesBorins commented Dec 25, 2018

Copy link
Copy Markdown
Contributor

This doesn't land cleanly on v11.x, should it be backported?

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
trace_events: move SetupTraceCategoryState into node_trace_events.cc
3d87d67 
It makes more sense to put it in `internalBinding('trace_events')`
instead of in the bootstrapper object.

PR-URL: nodejs#25128
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax commented Jan 14, 2019

Copy link
Copy Markdown
Member

This applies cleanly now.

addaleax pushed a commit that referenced this pull request Jan 14, 2019
trace_events: move SetupTraceCategoryState into node_trace_events.cc
19f1a50 
It makes more sense to put it in `internalBinding('trace_events')`
instead of in the bootstrapper object.

PR-URL: #25128
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
trace_events: move SetupTraceCategoryState into node_trace_events.cc
9a55bc8 
It makes more sense to put it in `internalBinding('trace_events')`
instead of in the bootstrapper object.

PR-URL: nodejs#25128
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Jan 24, 2019
Merged
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

jasnell jasnell approved these changes
richardlau richardlau approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL