Skip to content

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 13, 2017

I noticed that there are three static functions that only check if
args is a construct call. This commit suggests replacing them and
them with a lamda.

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

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Apr 13, 2017
@danbev
Copy link
Contributor Author

danbev commented Apr 13, 2017

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits. Typo in commit log: s/lamda/lambda/

Copy link
Member

Choose a reason for hiding this comment

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

Only two spaces of indent and call the variable is_construct_call (or is_construct_call_callback to indicate it's a function pointer.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah again, must remember to not use camelcase 😞 Thanks!

@danbev danbev force-pushed the cares_wrap-lamda branch from 3fc1684 to ce11999 Compare April 13, 2017 10:20
@danbev
Copy link
Contributor Author

danbev commented Apr 13, 2017

I noticed that there are three static functions that only check if
args is a construct call. This commit suggests replacing them and
them with a lambda.
@danbev danbev force-pushed the cares_wrap-lamda branch from ce11999 to a440ec8 Compare April 15, 2017 10:27
@danbev
Copy link
Contributor Author

danbev commented Apr 15, 2017

@addaleax
Copy link
Member

Landed in b280363

@addaleax addaleax closed this Apr 15, 2017
addaleax pushed a commit that referenced this pull request Apr 15, 2017
I noticed that there are three static functions that only check if
args is a construct call. This commit suggests replacing them and
them with a lambda.

PR-URL: #12384
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev danbev deleted the cares_wrap-lamda branch April 18, 2017 12:32
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

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++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants