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.

Sign up for GitHub

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

Jump to bottom

crypto: remove useless if statement #15041

Closed
wants to merge 1 commit into from
Closed

crypto: remove useless if statement #15041

wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Aug 26, 2017 •
edited
Loading

The if statement in ECDH.getPublicKey is useless. This change is to remove it.

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

crypto

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 26, 2017
@starkwang starkwang mentioned this pull request Aug 26, 2017
Closed
2 tasks
BridgeAR
BridgeAR approved these changes Aug 26, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

bnoordhuis
bnoordhuis requested changes Aug 26, 2017
View reviewed changes
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.

The if statement is not useless but there is a bug in the if statement below; it should have been else if, i.e., it needs to be a chain.

@BridgeAR
Copy link
Member

In that case there should also be a test case included.

@starkwang
Copy link
Contributor Author

@bnoordhuis In the docs for ecdh.getPublicKey([encoding][, format]), the encoding and format arguments must be of type undefined or string.
The if statement just assigns a value to f when receiving a format in type number, and then throws an TypeError. Assigning a value to f doesn't make any sense here.

@bnoordhuis
Copy link
Member

@indutny You are the author of those lines. What was the intent here?

@indutny
Copy link
Member

indutny commented Sep 2, 2017

Passing raw number down there when needed?

@BridgeAR
Copy link
Member

@indutny I am not sure I can follow your comment. Can you please elaborate? Right now this code path never worked because it always resulted in an error. Was this by accident and we miss a test case for this or should the if statement just be removed?

@BridgeAR
Copy link
Member

Ping @indutny again

@BridgeAR
Copy link
Member

@nodejs/tsc PTAL. I recommend removing this statement if we can not determine what it is meant for.

@indutny
Copy link
Member

indutny commented Sep 22, 2017

Sorry for cryptic comment. I didn't have enough time to elaborate that day.

The idea was that people may want to pass constant's raw numeric value into that function. Not sure if we want to have this API anymore, very likely we don't. It is going to be a major change, though.

@indutny indutny added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2017
@BridgeAR
Copy link
Member

@indutny this never worked, so it is definitely not a semver-major. If a user would have passed in a numeric value a error would have been thrown. It was also never documented at all.

As you say we do not want to support this anymore I guess we are safe to merge this.

@BridgeAR BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2017
@BridgeAR
Copy link
Member

@bnoordhuis I guess it is clear now that it is fine to land this?

@indutny
Copy link
Member

indutny commented Sep 23, 2017

Oh, you're right. LGTM then.

indutny
indutny approved these changes Sep 23, 2017
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

Wonderful!

@starkwang would you be so kind and rebase? This could land otherwise.

@BridgeAR
Copy link
Member

Ping @starkwang

@starkwang
crypto: remove useless if statement
12424fa 
The if statement in `ECDH.getPublicKey` is useless. This change
is to remove it.
@starkwang
Copy link
Contributor Author

@BridgeAR Sorry for my sluggishness. I've just rebased.

bnoordhuis
bnoordhuis approved these changes Sep 29, 2017
View reviewed changes
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. CI: https://ci.nodejs.org/job/node-test-pull-request/10339/

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Landed in 43e7e8d

@BridgeAR BridgeAR closed this Oct 2, 2017
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
@starkwang @BridgeAR
crypto: remove useless if statement
43e7e8d 
The if statement in `ECDH.getPublicKey` is useless. This change
is to remove it.

PR-URL: #15041
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins
Copy link
Contributor

Diffie helman isn't on 8.x afaik. Please feel free to change the labels if neccessary

addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
@starkwang @addaleax
crypto: remove useless if statement
3770047 
The if statement in `ECDH.getPublicKey` is useless. This change
is to remove it.

PR-URL: nodejs/node#15041
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@starkwang @BridgeAR @bnoordhuis @indutny @MylesBorins @nodejs-github-bot