Skip to content

src: make StreamBase::GetAsyncWrap pure virtual - #13174

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:stream-wrap-virtual-gaw
Closed

src: make StreamBase::GetAsyncWrap pure virtual#13174
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:stream-wrap-virtual-gaw

Conversation

@addaleax

@addaleaxaddaleax commented May 23, 2017

Copy link
Copy Markdown
Member

Splitting out from #13142, this is a bit that should be easy to review. (All remaining GetAsyncWrap methods never return nullptr.)

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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 23, 2017
@addaleaxaddaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2017

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

LGTM

@addaleax

Copy link
Copy Markdown
MemberAuthor

@addaleax

Copy link
Copy Markdown
MemberAuthor

Landed in 169c07d

@addaleax
addaleax deleted the stream-wrap-virtual-gaw branch May 25, 2017 18:02
addaleax added a commit that referenced this pull request May 25, 2017
PR-URL: #13174
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
jasnell pushed a commit that referenced this pull request May 25, 2017
PR-URL: #13174
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
jasnell pushed a commit that referenced this pull request May 28, 2017
PR-URL: #13174
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@jasnelljasnell mentioned this pull request May 28, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorinsMylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017
@MylesBorins

Copy link
Copy Markdown
Contributor

@addaleax should we land this now or wait for a larger async_hooks update?

@addaleaxaddaleax removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017
@addaleax

Copy link
Copy Markdown
MemberAuthor

@MylesBorins This is not the kind of change that would be harmful to include, or that would need to bake for any significant time. If it compiles, include it. :)

@MylesBorins

Copy link
Copy Markdown
Contributor

@addaleax it does not land cleanly, I don't think the async_wrap stuff has been backported

@MylesBorins

Copy link
Copy Markdown
Contributor

ping re: backport

Sign up for freeto 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++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@addaleax@MylesBorins@danbev@jasnell@AndreasMadsen@cjihrig@mscdex@nodejs-github-bot