★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18467Nouvelle recherche | Portail wanayoo
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

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

@ofrobots ofrobots commented Jan 30, 2018

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

@ofrobots ofrobots requested review from AndreasMadsen and addaleax Jan 30, 2018
AndreasMadsen
AndreasMadsen approved these changes Jan 30, 2018
View changes
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

LGTM and huge thanks from me.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 1, 2018

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

@AndreasMadsen
Copy link
Member

@AndreasMadsen AndreasMadsen commented Feb 1, 2018

@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 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 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 Feb 1, 2018
MylesBorins added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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
Member

@MylesBorins MylesBorins commented Mar 20, 2018

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

MylesBorins added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit to MayaLekova/node that referenced this issue 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

@AndreasMadsen AndreasMadsen

@cjihrig cjihrig

@BridgeAR BridgeAR

@addaleax addaleax

Assignees
No one assigned
Labels
async_hooks c++ fast-track
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

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