Skip to content

src: clean up stream_base.h and stream-base-inl.h - #32307

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:cleanup-streambase
Closed

src: clean up stream_base.h and stream-base-inl.h#32307
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:cleanup-streambase

Conversation

@jasnell

Copy link
Copy Markdown
Member

stream_base.h and stream_base-inl.h have always been a bit of a mess with regards to inlines, v8:: scopes, and more. This is the first step at a cleanup.

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

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 16, 2020
@jasnell
jasnellforce-pushed the cleanup-streambase branch from 0f6ef69 to ac42fe6CompareMarch 16, 2020 21:15
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnellforce-pushed the cleanup-streambase branch from ff51b55 to 098cbcfCompareMarch 16, 2020 21:39
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2020
@jasnell

Copy link
Copy Markdown
MemberAuthor

@nodejs/collaborators would be awesome to get another review on this one if possible.

Comment threadsrc/stream_wrap.cc
using v8::Local;
using v8::MaybeLocal;
using v8::Object;
using v8::PropertyAttribute;

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.

addtional for quic?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No, there's one use of PropertyAttribute elsewhere in the file. This was previously being inherited from the stream_base.h headers.

@gengjiawengengjiawen 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

jasnell added a commit that referenced this pull request Mar 19, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #32307
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@jasnell

Copy link
Copy Markdown
MemberAuthor

Landed in ffdf1de

@jasnelljasnell closed this Mar 19, 2020
MylesBorins pushed a commit that referenced this pull request Mar 19, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #32307
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 19, 2020
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #32307
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@targostargos added backport-blocked-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 22, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jasnell@nodejs-github-bot@addaleax@gengjiawen@targos