Skip to content
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,crypto: refactor crypto_tls.* #40675

Merged

Conversation

RaisinTen
Copy link
Contributor

By the design of GetSSLError(), the V8 API was unnecessarily being
accessed in places where it eventually didn't get used. So this refactor
inlines the function appropriately in places where it was being used.
Also, this replaces uses of AllocatedBuffers with BackingStores.

Signed-off-by: Darshan Sen darshan.sen@postman.com

@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. needs-ci PRs that need a full CI run. labels Oct 31, 2021
@nodejs-github-bot

This comment has been minimized.

@RaisinTen RaisinTen force-pushed the src,crypto/refactor-crypto_tls branch 4 times, most recently from 73bce22 to 261229d Compare November 1, 2021 15:59
src/crypto/crypto_tls.cc Outdated Show resolved Hide resolved
src/crypto/crypto_tls.cc Outdated Show resolved Hide resolved
src/crypto/crypto_tls.cc Outdated Show resolved Hide resolved
By the design of `GetSSLError()`, the V8 API was unnecessarily being
accessed in places where it eventually didn't get used. So this refactor
inlines the function appropriately in places where it was being used.
Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@RaisinTen RaisinTen force-pushed the src,crypto/refactor-crypto_tls branch from 261229d to 2ab44e9 Compare November 2, 2021 15:07
@nodejs-github-bot

This comment has been minimized.

@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Nov 2, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2021

Commit Queue failed
- Loading data for nodejs/node/pull/40675
✔  Done loading data for nodejs/node/pull/40675
----------------------------------- PR info ------------------------------------
Title      src,crypto: refactor `crypto_tls.*` (#40675)
Author     Darshan Sen  (@RaisinTen)
Branch     RaisinTen:src,crypto/refactor-crypto_tls -> nodejs:master
Labels     crypto, c++, author ready
Commits    1
 - src,crypto: refactor `crypto_tls.*`
Committers 1
 - Darshan Sen 
PR-URL: https://github.com/nodejs/node/pull/40675
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40675
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 31 Oct 2021 15:15:39 GMT
   ✔  Approvals: 1
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/40675#pullrequestreview-795054510
   ✖  This PR needs to wait 6 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-11-03T13:44:50Z: https://ci.nodejs.org/job/node-test-pull-request/40684/
- Querying data for job/node-test-pull-request/40684/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1431079248

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 7, 2021
@RaisinTen RaisinTen removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 7, 2021
@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2021
@nodejs-github-bot nodejs-github-bot merged commit 8ee9e1a into nodejs:master Nov 12, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 8ee9e1a

@RaisinTen RaisinTen deleted the src,crypto/refactor-crypto_tls branch November 13, 2021 04:48
targos pushed a commit that referenced this pull request Nov 21, 2021
By the design of `GetSSLError()`, the V8 API was unnecessarily being
accessed in places where it eventually didn't get used. So this refactor
inlines the function appropriately in places where it was being used.
Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #40675
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
By the design of `GetSSLError()`, the V8 API was unnecessarily being
accessed in places where it eventually didn't get used. So this refactor
inlines the function appropriately in places where it was being used.
Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #40675
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
By the design of `GetSSLError()`, the V8 API was unnecessarily being
accessed in places where it eventually didn't get used. So this refactor
inlines the function appropriately in places where it was being used.
Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #40675
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs 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++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants