Skip to content

inspector: zero out structure members - #8536

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:zero_out
Closed

inspector: zero out structure members#8536
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:zero_out

Conversation

@eugeneo

@eugeneoeugeneo commented Sep 14, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

Change is internal to inspector.

Description of change

Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

This is an attempt at guessing a fix for #8155 - so far I was not able to repro it.

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Sep 14, 2016
@eugeneo

eugeneo commented Sep 14, 2016

Copy link
Copy Markdown
ContributorAuthor

@ofrobots

@ofrobotsofrobots mentioned this pull request Sep 14, 2016
Closed
@ofrobots

ofrobots commented Sep 15, 2016

Copy link
Copy Markdown
Contributor

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

bnoordhuis
bnoordhuis approved these changes Sep 15, 2016
View reviewed changes

@bnoordhuisbnoordhuis 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. Maybe make the structs DISALLOW_COPY_AND_ASSIGN while you're here.

@eugeneo

eugeneo commented Sep 15, 2016

Copy link
Copy Markdown
ContributorAuthor

I've added DISALLOW_COPY_AND_ASSIGN. Then it showed that inspector socket instance needs a cleanup to be reused - so I decided that it would be best if inspector_accept did a reset, instead of expecting a clean instance.

Please take another look.

@ofrobots

ofrobots commented Sep 15, 2016

Copy link
Copy Markdown
Contributor

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

@eugeneo

eugeneo commented Sep 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Sorry, forgot to run the linter. I uploaded a fixed version.

@ofrobots

ofrobots commented Sep 15, 2016

Copy link
Copy Markdown
Contributor

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

bnoordhuis
bnoordhuis requested changes Sep 16, 2016
View reviewed changes
Comment threadsrc/inspector_socket.cc Outdated

@bnoordhuisbnoordhuisSep 16, 2016

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.

This looks like it should be a inspector_socket_s method.

@eugeneoeugeneoSep 16, 2016

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.

Thank you for the review. I introduced the function... But that pushed the struct to class territory too far for me :)

I changed the struct to class, renamed it appropriately and added the namespace. In fairness, other inspector_* functions need to become member functions as well and the state needs to be made private, etc.

But I am currently working on a complex refactoring that will significantly change this class (I'm hoping to remove libuv interactions) so I don't see much value in doing a comprehensive change right now...

Please take a look at updated CL.

bnoordhuis
bnoordhuis requested changes Sep 19, 2016
View reviewed changes
Comment threadsrc/inspector_socket.cc Outdated

@bnoordhuisbnoordhuisSep 19, 2016

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.

Maybe switch to static_cast while here?

@eugeneoeugeneoSep 19, 2016

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.

Done

Comment threadsrc/inspector_socket.h Outdated

@bnoordhuisbnoordhuisSep 19, 2016

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.

Indentation is off here.

@eugeneoeugeneoSep 19, 2016

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.

Done.

Comment threadsrc/inspector_socket.h Outdated

@bnoordhuisbnoordhuisSep 19, 2016

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.

Can you drop the struct keywords?

@eugeneoeugeneoSep 19, 2016

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.

Done

Comment threadtest/cctest/test_inspector_socket.cc Outdated

@bnoordhuisbnoordhuisSep 19, 2016

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.

static_cast?

@eugeneoeugeneoSep 19, 2016

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.

Done.

@eugeneo

eugeneo commented Sep 19, 2016

Copy link
Copy Markdown
ContributorAuthor

Thank you for the review. I updated the code, please take another look.

bnoordhuis
bnoordhuis approved these changes Sep 20, 2016
View reviewed changes

@bnoordhuisbnoordhuis 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. CI: https://ci.nodejs.org/job/node-test-pull-request/4144/

Eugene Ostroukhov
inspector: zero out structure members
39b9d13
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.
Attemp at fixing #8155 (so far I was not able to repro it)
@ofrobots

ofrobots commented Sep 20, 2016

Copy link
Copy Markdown
Contributor

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/4159/

@ofrobots

ofrobots commented Sep 20, 2016

Copy link
Copy Markdown
Contributor

Thanks, landed as bc1ebd6.

ofrobots pushed a commit that referenced this pull request Sep 20, 2016
Eugene Ostroukhov@ofrobots
inspector: zero out structure members
bc1ebd6
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.
Attempt at fixing #8155 (so far I was not able to repro it)
PR-URL: #8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
Eugene OstroukhovMyles Borins
inspector: zero out structure members
06e782f
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.
Attempt at fixing nodejs#8155 (so far I was not able to repro it)
PR-URL: nodejs#8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Eugene OstroukhovMyles Borins
inspector: zero out structure members
b05ce84
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.
Attempt at fixing #8155 (so far I was not able to repro it)
PR-URL: #8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo
eugeneo deleted the zero_out branch October 13, 2016 23:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eugeneo@ofrobots@bnoordhuis@MylesBorins@nodejs-github-bot