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 duplicate field env in CryptoJob class #31554

Closed
wants to merge 4 commits into from
Closed

src: remove duplicate field env in CryptoJob class #31554

wants to merge 4 commits into from

Conversation

DavenportEmma
Copy link
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
Collaborator

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
src/node_internals.h Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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
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
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
Collaborator

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

addaleax
addaleax approved these changes Feb 2, 2020
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’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
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
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

@devnexen devnexen devnexen approved these changes

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