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

src: make AsyncWrap constructors delegate #19366

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 15, 2018

Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.

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

Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 15, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 15, 2018

@danbev
Copy link
Contributor Author

danbev commented Mar 18, 2018

Landed in 75ff301.

@danbev danbev closed this Mar 18, 2018
@danbev danbev deleted the async_wrap_constructors branch March 18, 2018 11:07
danbev added a commit that referenced this pull request Mar 18, 2018
Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.

PR-URL: #19366
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.

PR-URL: #19366
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.

PR-URL: #19366
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Mar 20, 2018
codebytere pushed a commit that referenced this pull request Oct 18, 2018
Currently, there is an AsyncWrap constructor that is only used by
PromiseWrap. This constructor has a body which is very similar
to the other AsyncWrap constructor.

This commit suggests updating the private constructor that is used
by PromiseWrap and also have the second constructor delegate to this
one to avoid the code duplication.

PR-URL: #19366
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants