Skip to content

src: refactor grow_async_ids_stack#23808

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
Merged
refack merged 2 commits into from
Oct 24, 2018
Merged

src: refactor grow_async_ids_stack #23808

refack merged 2 commits into from
Oct 24, 2018

Conversation

@refack
Copy link
Contributor

@refackrefack commented Oct 21, 2018•
edited
Loading

Added AliasedBuffer::reserve()

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 21, 2018

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1327/pipeline

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 21, 2018
@refackrefack mentioned this pull request Oct 21, 2018
Merged
2 tasks
@refackrefack self-assigned this Oct 21, 2018
joyeecheung
joyeecheung reviewed Oct 23, 2018
View reviewed changes
addaleax
addaleax reviewed Oct 23, 2018
View reviewed changes
src/aliased_buffer.h Outdated
Copy link
Member

@addaleaxaddaleaxOct 22, 2018

Choose a reason for hiding this comment

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

style nit: old_size_in_bytes, new_size_in_bytes

Copy link
ContributorAuthor

@refackrefackOct 23, 2018

Choose a reason for hiding this comment

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

Ack.
Turned up the level of my IDE's inspection of these to "error".

src/aliased_buffer.h Outdated
Copy link
Member

@addaleaxaddaleaxOct 22, 2018

Choose a reason for hiding this comment

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

style nit: NativeT* new_buffer

Copy link
ContributorAuthor

@refackrefackOct 23, 2018

Choose a reason for hiding this comment

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

Ack

joyeecheung
joyeecheung approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

@joyeecheungjoyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with the style nits from @addaleax

@refack
Copy link
ContributorAuthor

refack commented Oct 23, 2018

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

@mmarchinimmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2018
@refackrefackforce-pushed the grow_async_ids branch 2 times, most recently from 888b26e to b845fd6CompareOctober 24, 2018 16:18
@refack
Copy link
ContributorAuthor

refack commented Oct 24, 2018

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

@refack
src: add AliasedBuffer::reserve
4044b06
refactor grow_async_ids_stack PR-URL: nodejs#23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@refack
src: add direct dependency on *-inl.h file
586daae
PR-URL: nodejs#23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@refackrefack merged commit 586daae into nodejs:masterOct 24, 2018
@refackrefack deleted the grow_async_ids branch October 24, 2018 21:37
@refackrefack removed their assignment Oct 24, 2018
targos pushed a commit that referenced this pull request Oct 26, 2018
@refack@targos
src: add AliasedBuffer::reserve
98ffcde
refactor grow_async_ids_stack PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
targos pushed a commit that referenced this pull request Oct 26, 2018
@refack@targos
src: add direct dependency on *-inl.h file
2e7189b
PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@targostargos mentioned this pull request Oct 27, 2018
Merged
@addaleaxaddaleax mentioned this pull request Oct 29, 2018
Closed
@targos
Copy link
Member

targos commented Nov 1, 2018

Marking dont-land-on-v11.x until nodejs/build#1542 is fixed

@refack
Copy link
ContributorAuthor

refack commented Nov 1, 2018

@targos there are two more PR that will require GCC4.9

@targostargos mentioned this pull request Nov 2, 2018
Closed
2 tasks
@refack
Copy link
ContributorAuthor

refack commented Nov 15, 2018

@targos the arm7 issue in ci-release was resolved - https://ci-release.nodejs.org/job/iojs+release/nodes=centos7-arm64-gcc6/3919/

targos pushed a commit that referenced this pull request Nov 18, 2018
@refack@targos
src: add AliasedBuffer::reserve
d0ab9d2
refactor grow_async_ids_stack PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
targos pushed a commit that referenced this pull request Nov 18, 2018
@refack@targos
src: add direct dependency on *-inl.h file
a8ccf47
PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@targos
Copy link
Member

targos commented Nov 18, 2018

@refack thanks! I backported this (and others) to v11.x and confirmed: https://ci-release.nodejs.org/job/iojs+release/3934/nodes=centos7-arm64-gcc6/

rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack@rvagg
src: add AliasedBuffer::reserve
33e7f6e
refactor grow_async_ids_stack PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack@rvagg
src: add direct dependency on *-inl.h file
25a9eee
PR-URL: #23808 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax left review comments

@joyeecheungjoyeecheungjoyeecheung approved these changes

+1 more reviewer

@mmarchinimmarchinimmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs 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.

7 participants

@refack@nodejs-github-bot@targos@addaleax@mmarchini@joyeecheung@codebytere