-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: move evp stuff to ncrypto #54911
Conversation
Review requested:
|
caf2971
to
143b9f4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54911 +/- ##
=======================================
Coverage 88.23% 88.24%
=======================================
Files 652 652
Lines 183856 183816 -40
Branches 35856 35841 -15
=======================================
- Hits 162229 162208 -21
+ Misses 14903 14888 -15
+ Partials 6724 6720 -4
|
if (!error.IsEmpty()) env->isolate()->ThrowException(error); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just return at this point, since 857 is returning true.
default: | ||
return false; | ||
} | ||
return key.id() == EVP_PKEY_ED25519 || key.id() == EVP_PKEY_ED448; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these functions can be constexpr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separately go through and make these kinds of improvements in one of the follow up PRs
@jasnell I feel that all these "move * to ncrypto" PRs that move stuff into a flat file structure are a step back in terms of the crypto subsystem refactoring that you've done in #35093. Do you plan on introducing a structure to it?
|
Yes, once things are moved over to ncrypto, the plan is to restructure ncrypto to makes things cleaner and break things up. It's a bit cumbersome right now given the sheer size of the task and trying to break it down into digestable chunks... but I promise it will come out improved when we're done. |
143b9f4
to
0197af6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
0197af6
to
3f5d3ca
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
PR-URL: #54911 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Landed in c4681d5 |
PR-URL: #54911 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs#54911 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs#54911 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
More incremental moving of crypto stuff to ncrypto ... there's a lot to so I'm chunking it up into smaller, more easily reviewed pieces.