FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: remove useless if statement by starkwang · Pull Request #15041 · nodejs/node · GitHub

/ node Public

crypto: remove useless if statement - #15041

Closed
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:crypto-rm-if
ConversationCommits (1)ChecksFiles changed
Closed

crypto: remove useless if statement#15041
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:crypto-rm-if

Conversation

starkwang commented Aug 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the crypto Issues and PRs related to the crypto subsystem. label Aug 26, 2017
starkwang mentioned this pull request Aug 26, 2017
Closed
2 tasks
BridgeAR approved these changes Aug 26, 2017
View reviewed changes

BridgeAR 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

bnoordhuis requested changes Aug 26, 2017
View reviewed changes

bnoordhuis 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 26, 2017

Copy link
Copy Markdown
Member

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

starkwang commented Aug 26, 2017

Copy link
Copy Markdown
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 commented Aug 28, 2017

Copy link
Copy Markdown
Member

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

indutny commented Sep 2, 2017

Copy link
Copy Markdown
Member

Passing raw number down there when needed?

BridgeAR commented Sep 11, 2017

Copy link
Copy Markdown
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 commented Sep 20, 2017

Copy link
Copy Markdown
Member

Ping @indutny again

BridgeAR commented Sep 22, 2017

Copy link
Copy Markdown
Member

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

indutny commented Sep 22, 2017

Copy link
Copy Markdown
Member

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 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2017

BridgeAR commented Sep 22, 2017

Copy link
Copy Markdown
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 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2017

BridgeAR commented Sep 22, 2017

Copy link
Copy Markdown
Member

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

indutny commented Sep 23, 2017

Copy link
Copy Markdown
Member

Oh, you're right. LGTM then.

indutny approved these changes Sep 23, 2017
View reviewed changes

indutny 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

BridgeAR commented Sep 23, 2017

Copy link
Copy Markdown
Member

Wonderful!

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

BridgeAR commented Sep 27, 2017

Copy link
Copy Markdown
Member

Ping @starkwang

crypto: remove useless if statement
12424fa 
The if statement in `ECDH.getPublicKey` is useless. This change
is to remove it.

starkwang commented Sep 29, 2017

Copy link
Copy Markdown
Contributor Author

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

bnoordhuis approved these changes Sep 29, 2017
View reviewed changes

bnoordhuis 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/10339/

BridgeAR commented Oct 2, 2017

Copy link
Copy Markdown
Member

Landed in 43e7e8d

BridgeAR closed this Oct 2, 2017
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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 <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

MylesBorins commented Oct 3, 2017

Copy link
Copy Markdown
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
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 <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
jasnell mentioned this pull request Oct 30, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
indutny indutny approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL