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.

Sign up for GitHub

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

Jump to bottom

src: remove unnecessary comment and add a CHECK in crypto_tls.cc #39991

Closed
wants to merge 1 commit into from
Closed

src: remove unnecessary comment and add a CHECK in crypto_tls.cc #39991

wants to merge 1 commit into from

Conversation

RaisinTen
Copy link
Contributor

Signed-off-by: Darshan Sen [email protected]

@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 Sep 4, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
addaleax
addaleax requested changes Sep 5, 2021
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t really see the point of the TODO comment. Inlining the Buffer::New() function gains us nothing and makes the code more complex. Switching to a plain Uint8Array, as the comment indicates, would be API breakage, and in particular API breakage that almost certainly isn’t worth ever actually doing.

src/crypto/crypto_tls.cc Show resolved Hide resolved
src/crypto/crypto_tls.cc Outdated Show resolved Hide resolved
@RaisinTen RaisinTen changed the title src: remove unnecessary uses of Buffer from crypto::KeylogCallback() src: remove unnecessary comment and add a CHECK in crypto_tls.cc Sep 10, 2021
@RaisinTen RaisinTen force-pushed the src/remove-unnecessary-uses-of-Buffer-from-KeylogCallback branch from 5060505 to 9a39eb5 Compare September 10, 2021 15:19
@RaisinTen
Copy link
Contributor Author

Inlining the Buffer::New() function gains us nothing and makes the code more complex.

@addaleax AllocatedBuffer is also just a utility wrapper around v8::BackingStore. Isn't inlining that also something we should not be aiming to do as it makes the code more complex? Refs: #39941 (number of lines of code went up a bit)

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@addaleax
Copy link
Member

Inlining the Buffer::New() function gains us nothing and makes the code more complex.

@addaleax AllocatedBuffer is also just a utility wrapper around v8::BackingStore. Isn't inlining that also something we should not be aiming to do as it makes the code more complex? Refs: #39941 (number of lines of code went up a bit)

Right, I am saying that I don’t see the need to use either AllocatedBuffer or v8::BackingStore when we’re just going to create a Buffer anyway. I guess I viewed #39941 a bit differently because that is actually moving us towards getting rid of AllocatedBuffer, which is something that we should probably do in the long run. But here we weren’t even using AllocatedBuffer, we were using the Buffer API which is stable and never going away :)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39906/

@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 Sep 11, 2021
This was referenced Sep 14, 2021
Open
Open
Open
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2021
@github-actions
Copy link
Contributor

Landed in 341312d...604c1df

@github-actions github-actions bot closed this Sep 16, 2021
nodejs-github-bot pushed a commit that referenced this pull request Sep 16, 2021
@RaisinTen @nodejs-github-bot
src: remove unnecessary comment and add a CHECK in crypto_tls.cc
604c1df 
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #39991
Reviewed-By: Anna Henningsen <[email protected]>
This was referenced Sep 17, 2021
Open
Open
@RaisinTen RaisinTen deleted the src/remove-unnecessary-uses-of-Buffer-from-KeylogCallback branch September 18, 2021 07:47
This was referenced Sep 19, 2021
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@RaisinTen @BethGriggs
src: remove unnecessary comment and add a CHECK in crypto_tls.cc
0a7b50a 
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #39991
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@RaisinTen @BethGriggs
src: remove unnecessary comment and add a CHECK in crypto_tls.cc
95528b2 
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: #39991
Reviewed-By: Anna Henningsen <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RaisinTen @nodejs-github-bot @addaleax @aduh95