Skip to content

src: avoid strcmp in SecureContext::Init#34329

Closed
tniessen wants to merge 3 commits intonodejs:masterfrom
tniessen:src-avoid-strcmp-in-securecontext-init
Closed

src: avoid strcmp in SecureContext::Init#34329
tniessen wants to merge 3 commits intonodejs:masterfrom
tniessen:src-avoid-strcmp-in-securecontext-init

Conversation

@tniessen
Copy link
Member

@tniessen tniessen commented Jul 12, 2020

This change uses std::string::operator== instead of strcmp, and de-duplicates some code.

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

@nodejs-github-bot nodejs-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 Jul 12, 2020
@yashLadha
Copy link
Contributor

yashLadha commented Jul 13, 2020

Any particular reason to avoid using strcmp and use ==? Curious to know the underlying reason for the change.

yashLadha
yashLadha reviewed Jul 13, 2020
View reviewed changes
src/node_crypto.cc Outdated
Comment on lines 577 to 579
if (methodstr == "SSLv2_method" ||
methodstr == "SSLv2_server_method" ||
methodstr == "SSLv2_client_method") {
Copy link
Contributor

@yashLadha yashLadha Jul 13, 2020

Choose a reason for hiding this comment

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

Nice 👍

@tniessen
Copy link
Member Author

tniessen commented Jul 13, 2020

Any particular reason to avoid using strcmp and use ==? Curious to know the underlying reason for the change.

In my opinion, operator== is simpler, has clearer semantics than strcmp(...) == 0, and makes the code easier to understand. A dozen branches containing strcmp(...) == 0 also kind of violate the DRY concept.

bnoordhuis
bnoordhuis reviewed Jul 13, 2020
View reviewed changes
bnoordhuis
bnoordhuis approved these changes Jul 14, 2020
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.

👍

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 17, 2020

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

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 17, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 17, 2020

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

tniessen added a commit to tniessen/node that referenced this pull request Jul 19, 2020
@tniessen
src: avoid strcmp in SecureContext::Init
efd1667 
PR-URL: nodejs#34329
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@tniessen
Copy link
Member Author

tniessen commented Jul 19, 2020

Landed in efd1667, thank you for reviewing.

@tniessen tniessen closed this Jul 19, 2020
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 19, 2020
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
@tniessen @cjihrig
src: avoid strcmp in SecureContext::Init
5d5036d 
PR-URL: #34329
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 27, 2020
@tniessen @MylesBorins
src: avoid strcmp in SecureContext::Init
9d40af5 
PR-URL: #34329
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jul 28, 2020
Merged
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@tniessen @addaleax
src: avoid strcmp in SecureContext::Init
c5b6c69 
PR-URL: #34329
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@tniessen @addaleax
src: avoid strcmp in SecureContext::Init
5a16a67 
PR-URL: #34329
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
@mhdawson mhdawson mentioned this pull request Jan 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@yashLadha yashLadha yashLadha left review comments

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

5 participants

@tniessen @yashLadha @nodejs-github-bot @bnoordhuis @jasnell