Skip to content

crypto: remove unused code in sign/verify - #12397

Closed
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-cleanup
Closed

crypto: remove unused code in sign/verify#12397
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-cleanup

Conversation

@tniessen

@tniessentniessen commented Apr 13, 2017

Copy link
Copy Markdown
Member

Removes unused code in node_crypto.cc in Sign::SignFinal and Verify::VerifyFinal as suggested by @addaleaxhere.

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

crypto

@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 Apr 13, 2017
sam-github
sam-github suggested changes Apr 13, 2017
View reviewed changes

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks ok if its passing tests, but the commit message could say something more specific. Was there an encoding arg supported by the C++ layer that was always being passed null, and you removed that?

addaleax
addaleax approved these changes Apr 13, 2017
View reviewed changes

@addaleaxaddaleax 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.

Looks good, but I agree that the commit message could be a bit more specific

@tniessen

tniessen commented Apr 13, 2017

Copy link
Copy Markdown
MemberAuthor

@sam-github@addaleax I tried to do better this time. Yes, it is about an unused encoding parameter in both methods.

sam-github
sam-github approved these changes Apr 13, 2017
View reviewed changes

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, nice commit message. Maybe subject could be more specific, like "crypto: remove unused C++ arg in sign/verify" if its < 50 chars, but I'm good with this message as-is.

@tniessen
crypto: remove unused C++ parameter in sign/verify
1d9aa7a
Removes code in node_crypto.cc in Sign::SignFinal and
Verify::VerifyFinal which allowed to convert between buffers and
strings based on given encodings. The code is unused as crypto.js
only passes in and expects buffers and does the conversion itself.
The encoding parameter was removed from both methods.
@sam-github

sam-github commented Apr 13, 2017

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/7378/

@tniessen

tniessen commented Apr 13, 2017

Copy link
Copy Markdown
MemberAuthor

@sam-github I changed the subject to "crypto: remove unused C++ parameter in sign/verify" (50 chars) but will wait for CI to finish after experiencing problems with CI and force-pushs.

@tniessen

tniessen commented Apr 13, 2017

Copy link
Copy Markdown
MemberAuthor

CI passed, subject changed.

jasnell
jasnell approved these changes Apr 13, 2017
View reviewed changes

@jasnelljasnell 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 if CI is passing.

@jasnell

jasnell commented Apr 18, 2017

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/7488/

jasnell pushed a commit that referenced this pull request Apr 18, 2017
@tniessen@jasnell
crypto: remove unused C++ parameter in sign/verify
eaa0542
Removes code in node_crypto.cc in Sign::SignFinal and
Verify::VerifyFinal which allowed to convert between buffers and
strings based on given encodings. The code is unused as crypto.js
only passes in and expects buffers and does the conversion itself.
The encoding parameter was removed from both methods.
PR-URL: #12397
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

jasnell commented Apr 18, 2017

Copy link
Copy Markdown
Member

Landed in eaa0542

@jasnelljasnell closed this Apr 18, 2017
@jasnelljasnell mentioned this pull request May 11, 2017
Closed
@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn

gibfahn commented Jun 18, 2017

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

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@sam-githubsam-githubsam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tniessen@sam-github@jasnell@gibfahn@addaleax@cjihrig@nodejs-github-bot