Skip to content

crypto: replace gotos #23132

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

tniessen
Copy link
Member

These occurrences of goto are almost trivial and only end up calling End(). I personally don't think it makes the code less expressive, but I'd like to know if someone feels different.

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

@nodejs-github-bot
Copy link
Collaborator

@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 Sep 27, 2018
@tniessen
Copy link
Member Author

tniessen commented Sep 27, 2018

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.

If someone is feeling adventurous: I'd be curious to know if the custom handshake parser still wins out from openssl.

@tniessen
Copy link
Member Author

tniessen commented Sep 28, 2018

This conflicts with #23014. I kind of like this variant better, but I can close it if others don't feel the same. I'd prefer to use OnScopeLeave only when necessary to cleanup resources, which isn't the case here in my opinion.

@gireeshpunathil
Copy link
Member

@tniessen - I was excited by the power of combining scoped variables. destructors and lambda, to tidy up the code and probably bring-in some functional programming elements to it when I developed #23014 . To me, other than the fact that the trapped object being destructed when leaving the scope (and leverage that destructor to run custom task on return), I don't see any connection with resource clean-up here.

If you think this is more readable and efficient, I am not objecting. thanks!

@tniessen tniessen force-pushed the crypto-replace-gotos branch from 158c3eb to 0120eb1 Compare September 28, 2018 13:17
@tniessen
Copy link
Member Author

Rebased, old HEAD was 158c3eb063ba9983dfaca0067bf87455000f695e. @gireeshpunathil Thanks. I still liked your PR, I even approved it, I just forgot about it when I opened this one.

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 30, 2018
@addaleax
Copy link
Member

addaleax commented Oct 1, 2018

@tniessen
Copy link
Member Author

tniessen commented Oct 1, 2018

@tniessen
Copy link
Member Author

tniessen commented Oct 1, 2018

Landed in 640172d, thanks for reviewing.

@tniessen tniessen closed this Oct 1, 2018
tniessen added a commit that referenced this pull request Oct 1, 2018
PR-URL: #23132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #23132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.