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.

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

src: revamp internal field count tracking #31960

Closed

Conversation

@jasnell
Copy link
Member

jasnell commented Feb 26, 2020

Improved handling of internal field counts.

In a separate PR, it would likely be good to switch to use of utility functions that include DCHECKs for the InternalFieldCount().

/cc @bnoordhuis @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@jasnell jasnell mentioned this pull request Feb 26, 2020
2 of 2 tasks complete
src/async_wrap.cc Outdated Show resolved Hide resolved
src/base_object-inl.h Outdated Show resolved Hide resolved
src/cares_wrap.cc Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jasnell:revamp-internal-field-count-tracking branch Feb 26, 2020
@jasnell jasnell requested a review from addaleax Feb 26, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

bnoordhuis left a comment

Thanks, this is already much better than the current state of things and looks like it already exposes a few infidelities and at least one outright bug.

src/async_wrap.cc Outdated Show resolved Hide resolved
src/base_object.h Outdated Show resolved Hide resolved
src/node_object_wrap.h Outdated Show resolved Hide resolved
src/stream_base.h Show resolved Hide resolved
src/stream_base.h Show resolved Hide resolved
src/stream_wrap.cc Outdated Show resolved Hide resolved
src/async_wrap.cc Outdated Show resolved Hide resolved
@addaleax addaleax mentioned this pull request Feb 26, 2020
2 of 2 tasks complete
@jasnell jasnell requested a review from addaleax Feb 26, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell requested a review from bnoordhuis Feb 26, 2020
Copy link
Member

bnoordhuis left a comment

Thanks, LGTM with suggestions.

src/stream_base-inl.h Outdated Show resolved Hide resolved
src/stream_base.h Outdated Show resolved Hide resolved
src/stream_base.h Outdated Show resolved Hide resolved
Copy link
Member

mcollina left a comment

lgtm

@nodejs-github-bot

This comment has been minimized.

Change suggested by bnoordhuis.

Improve handing of internal field counting by using enums.
Helps protect against future possible breakage if field
indexes are ever changed or added to.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell force-pushed the jasnell:revamp-internal-field-count-tracking branch to 9c6e9cd Feb 29, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

jasnell added a commit that referenced this pull request Mar 2, 2020
Change suggested by bnoordhuis.

Improve handing of internal field counting by using enums.
Helps protect against future possible breakage if field
indexes are ever changed or added to.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #31960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Mar 2, 2020

Landed in 31960

@jasnell jasnell closed this Mar 2, 2020
MylesBorins added a commit that referenced this pull request Mar 4, 2020
Change suggested by bnoordhuis.

Improve handing of internal field counting by using enums.
Helps protect against future possible breakage if field
indexes are ever changed or added to.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #31960
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.