Skip to content

Conversation

@danbev
Copy link
Contributor

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-botnodejs-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
ContributorAuthor

Copy link
Member

@bnoordhuisbnoordhuis 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
ContributorAuthor

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
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

@addaleax
Copy link
Member

Landed in b280363

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
@danbevdanbev deleted the cares_wrap-lamda branch April 18, 2017 12:32
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

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 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++.caresIssues 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

@danbev@addaleax@gibfahn@bnoordhuis@jasnell@cjihrig@nodejs-github-bot