Skip to content

crypto: simplify diffiehellman getFormat function - #20246

Closed
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:crypto_diffiehellman_js
Closed

crypto: simplify diffiehellman getFormat function#20246
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:crypto_diffiehellman_js

Conversation

@danbev

Copy link
Copy Markdown
Contributor

This commit aims to simplify the getFormat function in
diffiehellman.js.

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

This commit aims to simplify the getFormat function in
diffiehellman.js.
@nodejs-github-botnodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Apr 24, 2018
@danbev

Copy link
Copy Markdown
ContributorAuthor

Comment threadlib/internal/crypto/diffiehellman.js Outdated
else if (format === 'hybrid')
f = POINT_CONVERSION_HYBRID;
return POINT_CONVERSION_HYBRID;
// Default

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.

Can you remove this comment or move to line 231 as POINT_CONVERSION_UNCOMPRESSED is the default?

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 thanks, I'll update shortly.

Comment threadlib/internal/crypto/diffiehellman.js Outdated
if (format === 'compressed')
f = POINT_CONVERSION_COMPRESSED;
return POINT_CONVERSION_COMPRESSED;
else if (format === 'hybrid')

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.

The else becomes obsolete here. The same for the next one.

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.

Good point, I missed that. Will update shortly. Thanks

@danbev

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR

Copy link
Copy Markdown
Member

@tniessen@trivikr@cjihrig@jasnell it would be nice if you add the author-ready after giving your LG if there are no outstanding comments while giving a LG and a CI is running.

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@trivikr

Copy link
Copy Markdown
Member

@BridgeAR Noted! 👍

@danbev

Copy link
Copy Markdown
ContributorAuthor

Landed in 3b8ab2a.

@danbevdanbev closed this Apr 26, 2018
@danbev
danbev deleted the crypto_diffiehellman_js branch April 26, 2018 05:32
danbev added a commit to danbev/node that referenced this pull request Apr 26, 2018
This commit aims to simplify the getFormat function in
diffiehellman.js.
PR-URL: nodejs#20246
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
This commit aims to simplify the getFormat function in
diffiehellman.js.
PR-URL: #20246
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request May 8, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@danbev@BridgeAR@trivikr@jasnell@cjihrig@tniessen@nodejs-github-bot