Skip to content

Conversation

@jasnell
Copy link
Member

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
from node_crypto.cc to allow them to be reused. That's where this
commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
it's organization and lack of error handling and use of Local vs.
MaybeLocal. This cleans that up a bit and hopefully makes certain
parts a bit more manageable also.

There is nothing QUIC specific in this PR but there are pieces in here that are only used by the QUIC implementation currently. I am separating things out to make them easier to review in smaller chunks.

This PR does include a number of changes that are not yet in the QUIC version as I decided once I started separating it out to do some additional cleanup while I was in there.

These should all be internal changes with no changes to public API at all.

Signed-off-by: James M Snell [email protected]

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++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 29, 2020
@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]>
@jasnelljasnellforce-pushed the crypto-common-refactor branch from af1824a to e1b0c3dCompareMarch 2, 2020 22:09
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

jasnell added a commit that referenced this pull request Mar 3, 2020
Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]> PR-URL: #32016 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in dd81836

@jasnelljasnell closed this Mar 3, 2020
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]> PR-URL: #32016 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Mar 4, 2020
codebytere pushed a commit that referenced this pull request Mar 16, 2020
Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]> PR-URL: #32016 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]> PR-URL: #32016 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Two things in one on this commit: (a) For the QUIC implementation, we need to separate out various bits from node_crypto.cc to allow them to be reused. That's where this commit starts. (b) Quite a bit of the node_crypto.cc code was just messy in terms of it's organization and lack of error handling and use of Local vs. MaybeLocal. This cleans that up a bit and hopefully makes certain parts a bit more manageable also. Signed-off-by: James M Snell <[email protected]> PR-URL: #32016 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
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.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell@nodejs-github-bot@sam-github@addaleax