Skip to content

src: replace IsConstructCall functions with lamda - #12384

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:cares_wrap-lamda
Closed

src: replace IsConstructCall functions with lamda#12384
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:cares_wrap-lamda

Conversation

@danbev

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

@bnoordhuisbnoordhuis 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 modulo nits. Typo in commit log: s/lamda/lambda/

Comment threadsrc/cares_wrap.cc Outdated

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.

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

@addaleax

Copy link
Copy Markdown
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 <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
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn

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