Skip to content

src: refactor grow_async_ids_stack - #23808

Merged
refack merged 2 commits into
nodejs:masterfrom
refack:grow_async_ids
Oct 24, 2018
Merged

src: refactor grow_async_ids_stack#23808
refack merged 2 commits into
nodejs:masterfrom
refack:grow_async_ids

Conversation

@refack

@refackrefack commented Oct 21, 2018

Copy link
Copy Markdown
Contributor

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

@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 self-assigned this Oct 21, 2018
Comment threadsrc/aliased_buffer.h Outdated
Comment threadsrc/aliased_buffer.h Outdated
Comment threadsrc/aliased_buffer.h Outdated

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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".

Comment threadsrc/aliased_buffer.h Outdated

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ack

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

@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
@refack
refackforce-pushed the grow_async_ids branch 2 times, most recently from 888b26e to b845fd6CompareOctober 24, 2018 16:18
@refack

Copy link
Copy Markdown
ContributorAuthor

refactor grow_async_ids_stack
PR-URL: nodejs#23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
PR-URL: nodejs#23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@refack
refack merged commit 586daae into nodejs:masterOct 24, 2018
@refack
refack 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
refactor grow_async_ids_stack
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Oct 26, 2018
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@targostargos mentioned this pull request Oct 27, 2018
@addaleaxaddaleax mentioned this pull request Oct 29, 2018
@targos

Copy link
Copy Markdown
Member

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

@refack

Copy link
Copy Markdown
ContributorAuthor

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

@targostargos mentioned this pull request Nov 2, 2018
2 tasks
@refack

Copy link
Copy Markdown
ContributorAuthor

@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
refactor grow_async_ids_stack
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Nov 18, 2018
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@targos

Copy link
Copy Markdown
Member

@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
refactor grow_async_ids_stack
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23808
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

7 participants

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