Skip to content

src: remove duplicate field env in CryptoJob class#31554

Closed
DavenportEmma wants to merge 4 commits intonodejs:masterfrom
DavenportEmma:new-branch
Closed

src: remove duplicate field env in CryptoJob class#31554
DavenportEmma wants to merge 4 commits intonodejs:masterfrom
DavenportEmma:new-branch

Conversation

@DavenportEmma
Copy link
Copy Markdown
Contributor

@DavenportEmma DavenportEmma commented Jan 28, 2020 •
edited
Loading

Removed field env from cryptojob class, replaced with function get_env() inherited from ThreadPoolWork

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@DavenportEmma
src: remove duplicate field env in CryptoJob class
8ccb0aa 
Removed field env from cryptojob class, replaced with function get_env() inherited from ThreadPoolWork
@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. labels Jan 28, 2020
@jasnell jasnell requested a review from addaleax January 28, 2020 16:42
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Jan 28, 2020

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2020
bnoordhuis
bnoordhuis reviewed Jan 29, 2020
View reviewed changes
Comment thread src/node_internals.h Outdated
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Jan 31, 2020

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

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jan 31, 2020
@bnoordhuis
src: remove duplicate env field from CryptoJob
94935b2 
Add a `ThreadPoolWork::env()` getter and use that. To fix the diamond
problem in class CompressionStream that inherits from both AsyncWrap
and ThreadPoolWork, we add a `CompressionStream::env()` getter that
delegates to `AsyncWrap::env()`.

Refs: nodejs#31554 (comment)
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jan 31, 2020
@bnoordhuis
src: StreamBase::stream_env() -> StreamBase::env()
47c4eac 
Rename `StreamBase::stream_env()` to `StreamBase::env()` and work around
the diamond problem by adding `env()` getters to several classes.

Refs: nodejs#31554 (comment)
@bnoordhuis bnoordhuis mentioned this pull request Jan 31, 2020
Closed
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Jan 31, 2020 •
edited
Loading

I'm good with either this PR or @bnoordhuis' alternative #31588 landing (but have asked that @conordavenport be listed as a co-author on the alternative)

@conordavenport ... I'm working up an additional fixup commit for this that uses env() instead of get_env(). I'll push it up when it's ready.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Feb 1, 2020

@conordavenport ... ok, I pushed a new commit that should work but I haven't tested it on macos yet.

@addaleax @Trott @benjamingr @cjihrig @devnexen ... can I ask you to give this another lookover to see if it still looks good to you.

@jasnell

This comment has been minimized.

Sign in to view
1 similar comment
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Feb 1, 2020

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

addaleax
addaleax approved these changes Feb 2, 2020
View reviewed changes
Copy link
Copy Markdown
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’m also good with this, yes. (Kinda preferred the original variant of using a different method name, of all the ones out there, tbh.)

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Feb 2, 2020

CI Green on retry (https://ci.nodejs.org/job/node-test-commit-windows-fanned/33701/)

jasnell pushed a commit that referenced this pull request Feb 2, 2020
@DavenportEmma @jasnell
src: remove duplicate field env in CryptoJob class
99c8c6d 
Removed field env from cryptojob class, replaced with
function env() inherited from ThreadPoolWork

PR-URL: #31554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Feb 2, 2020

Landed in 99c8c6d

@jasnell jasnell closed this Feb 2, 2020
@DavenportEmma DavenportEmma deleted the new-branch branch February 7, 2020 10:01
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@DavenportEmma @codebytere
src: remove duplicate field env in CryptoJob class
42b8354 
Removed field env from cryptojob class, replaced with
function env() inherited from ThreadPoolWork

PR-URL: #31554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@DavenportEmma @codebytere
src: remove duplicate field env in CryptoJob class
03eedd8 
Removed field env from cryptojob class, replaced with
function env() inherited from ThreadPoolWork

PR-URL: #31554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@DavenportEmma @codebytere
src: remove duplicate field env in CryptoJob class
9e2ceab 
Removed field env from cryptojob class, replaced with
function env() inherited from ThreadPoolWork

PR-URL: #31554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@DavenportEmma @codebytere
src: remove duplicate field env in CryptoJob class
79a6872 
Removed field env from cryptojob class, replaced with
function env() inherited from ThreadPoolWork

PR-URL: #31554
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

+1 more reviewer

@devnexen devnexen devnexen approved these changes

Reviewers whose approvals may not affect merge requirements

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.

10 participants

@DavenportEmma @nodejs-github-bot @jasnell @bnoordhuis @Trott @addaleax @benjamingr @cjihrig @tniessen @devnexen