Skip to content

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

tniessen commented Sep 27, 2018

Copy link
Member

@bnoordhuisbnoordhuis 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
MemberAuthor

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
Copy link
MemberAuthor

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

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

@tniessen
Copy link
MemberAuthor

@tniessen
Copy link
MemberAuthor

Landed in 640172d, thanks for reviewing.

@tniessentniessen closed this Oct 1, 2018
tniessen added a commit that referenced this pull request Oct 1, 2018
PR-URL: #23132 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
PR-URL: #23132 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Oct 7, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

11 participants

@tniessen@nodejs-github-bot@gireeshpunathil@addaleax@bnoordhuis@jasnell@thefourtheye@lpinca@cjihrig@lundibundi@mhdawson