Skip to content

Conversation

@danbev
Copy link
Contributor

@danbevdanbev commented Dec 5, 2017

This commit updates the set_field function pointer to return an int, and
also updates the lambdas with a return statement.

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++. crypto Issues and PRs related to the crypto subsystem. labels Dec 5, 2017
@danbev
Copy link
ContributorAuthor

@bnoordhuis
Copy link
Member

The DH_set0_key() in src/node_crypto.cc is a backwards compatibility shim for the function of the same name in openssl 1.1.0, where it returns int.

@danbev
Copy link
ContributorAuthor

The DH_set0_key() in src/node_crypto.cc is a backwards compatibility shim for the function of the same name in openssl 1.1.0, where it returns int.

Ah I see that now, thanks for clarifying!

Is there a reason for not declaring the return type for the function pointer? I've added a commit with what I mean.

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 with a suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Since you're threading through the return value, you might as well CHECK_EQ(1, set_field(dh->dh, num));.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sounds good, I'll add the check. Thanks

This commit updates the set_field function pointer to return an int, and also updates the lambdas with a return statement.
@danbevdanbevforce-pushed the crypto_dh_set0_key_void branch from acbe9e6 to 0a1bfc1CompareDecember 7, 2017 06:55
@danbevdanbev changed the title crypto: make DH_set0_key voidcrypto: declare int return type for set_fieldDec 7, 2017
@danbev
Copy link
ContributorAuthor

@danbev
Copy link
ContributorAuthor

Landed in abc2801

@danbevdanbev closed this Dec 8, 2017
danbev added a commit that referenced this pull request Dec 8, 2017
This commit updates the set_field function pointer to return an int, and also updates the lambdas with a return statement. PR-URL: #17468 Reviewed-By: Ben Noordhuis <[email protected]>
@danbevdanbev deleted the crypto_dh_set0_key_void branch December 8, 2017 06:22
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit updates the set_field function pointer to return an int, and also updates the lambdas with a return statement. PR-URL: #17468 Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit updates the set_field function pointer to return an int, and also updates the lambdas with a return statement. PR-URL: #17468 Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

Should this be backported to v8.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++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@danbev@bnoordhuis@gibfahn@MylesBorins@nodejs-github-bot