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.

Sign up for GitHub

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

Jump to bottom

async_hooks: clean up comments #18467

Merged
merged 1 commit into from
Feb 1, 2018
Merged

async_hooks: clean up comments #18467

merged 1 commit into from
Feb 1, 2018

Conversation

ofrobots
Copy link
Contributor

With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04

Checklist
Affected core subsystem(s)

async_hooks

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jan 30, 2018
AndreasMadsen
AndreasMadsen approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM and huge thanks from me.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Feb 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

CI https://ci.nodejs.org/job/node-test-commit-light/201/

@AndreasMadsen
Copy link
Member

@BridgeAR What are the rules for fast-tracing PRs? I don't see any reasons for fast-tracking this as it doesn't solve any critical bugs (also 48 hours have already passed).

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

@AndreasMadsen on the Node Collaborator Summit we spoke about it and we could not find really hard rules for it. But trivial changes like doc things that already got multiple looking good are a good example. And due to the label others can disagree while the label is there (the label is stuck for a while as well).

@ofrobots
async_hooks: clean up comments
ad94be8 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: nodejs@83e5215
Ref: nodejs@0784b04
PR-URL: nodejs#18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ofrobots ofrobots merged commit ad94be8 into nodejs:master Feb 1, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented Feb 1, 2018

Thanks. Landed in ad94be8.

I intentionally wanted to keep this open for the full 2 days. AsyncHooks can be a fairly complex and subtle topic and the idea was to make sure there is nothing missing in the understanding of the semantics.

@ofrobots ofrobots deleted the async-hooks-comments branch February 1, 2018 17:59
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
b2eff72 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
550fdac 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
0865d11 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
Contributor

Landed on 8.x, please lmk if this shouldn't have

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
3b7b936 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
6b34958 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
cad0bbb 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@ofrobots @MylesBorins
async_hooks: clean up comments
ce3866b 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: 83e5215
Ref: 0784b04
PR-URL: #18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ofrobots @MayaLekova
async_hooks: clean up comments
a641ba5 
With some of the recent work, some of the comments were no longer
representative of the code, or were otherwise unclear. This commit
fixes some obvious issues I found.

Ref: nodejs@83e5215
Ref: nodejs@0784b04
PR-URL: nodejs#18467
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@AndreasMadsen AndreasMadsen AndreasMadsen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ofrobots @BridgeAR @AndreasMadsen @MylesBorins @jasnell @cjihrig @addaleax @nodejs-github-bot