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: separate out NgLibMemoryManagerBase #33104

Closed
wants to merge 2 commits into from
Closed

src: separate out NgLibMemoryManagerBase #33104

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 27, 2020 •
edited
Loading

Extracted from the QUIC PR

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

/cc @addaleax @sam-github

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

@jasnell
src: separate out NgLibMemoryManagerBase
3a0d6c4 
Extracted from the [QUIC PR](nodejs#32379)

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 27, 2020
@nodejs-github-bot
Copy link
Collaborator

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

addaleax
addaleax approved these changes Apr 27, 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.

LGTM, but this doesn't really make sense outside of the QUIC context. I think keeping it as a separate commit in the QUIC PR should be fine.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 28, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31067/ (💚)

@jasnell
Copy link
Member Author

jasnell commented Apr 28, 2020

I think keeping it as a separate commit in the QUIC PR should be fine.

Ordinarily I'd agree but the QUIC PR is already massive and difficult to review as is. Separating this out is not harmful and the change is small enough that it has no impact on other bits landing separately.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2020
jasnell added a commit that referenced this pull request Apr 29, 2020
@jasnell
src: separate out NgLibMemoryManagerBase
1d69c6f 
Extracted from the [QUIC PR](#32379)

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33104
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2020

Landed in 1d69c6f

@jasnell jasnell closed this Apr 29, 2020
targos pushed a commit that referenced this pull request May 4, 2020
@jasnell @targos
src: separate out NgLibMemoryManagerBase
541ea03 
Extracted from the [QUIC PR](#32379)

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33104
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@targos targos mentioned this pull request May 4, 2020
Merged
targos pushed a commit that referenced this pull request May 7, 2020
@jasnell @targos
src: separate out NgLibMemoryManagerBase
9350315 
Extracted from the [QUIC PR](#32379)

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33104
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@targos targos mentioned this pull request May 7, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
@jasnell @targos
src: separate out NgLibMemoryManagerBase
6b1e235 
Extracted from the [QUIC PR](#32379)

So far, this is only used by the QUIC PR directly but the change itself
is independent of QUIC, even if not used directly by anything else yet.
Separated out per request.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #33104
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
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

@sam-github sam-github sam-github 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++.
Projects
None yet
Milestone
No milestone
Development

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