Skip to content

inspector: zero out structure members#8536

Closed
eugeneo wants to merge 1 commit intonodejs:masterfrom
eugeneo:zero_out
Closed

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

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 14, 2016

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-bot nodejs-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
Copy link
Contributor Author

eugeneo commented Sep 14, 2016

@ofrobots

@ofrobots ofrobots mentioned this pull request Sep 14, 2016
Closed
@ofrobots
Copy link
Contributor

ofrobots commented Sep 15, 2016

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

bnoordhuis
bnoordhuis approved these changes Sep 15, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

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
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

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
Copy link
Contributor

ofrobots commented Sep 15, 2016

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

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

@ofrobots
Copy link
Contributor

ofrobots commented Sep 15, 2016

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

bnoordhuis
bnoordhuis requested changes Sep 16, 2016
View reviewed changes
src/inspector_socket.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Sep 16, 2016

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.

Copy link
Contributor Author

@eugeneo eugeneo Sep 16, 2016

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
src/inspector_socket.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016

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?

Copy link
Contributor Author

@eugeneo eugeneo Sep 19, 2016

Choose a reason for hiding this comment

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

Done

src/inspector_socket.h Outdated
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016

Choose a reason for hiding this comment

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

Indentation is off here.

Copy link
Contributor Author

@eugeneo eugeneo Sep 19, 2016

Choose a reason for hiding this comment

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

Done.

src/inspector_socket.h Outdated
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016

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?

Copy link
Contributor Author

@eugeneo eugeneo Sep 19, 2016

Choose a reason for hiding this comment

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

Done

test/cctest/test_inspector_socket.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016

Choose a reason for hiding this comment

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

static_cast?

Copy link
Contributor Author

@eugeneo eugeneo Sep 19, 2016

Choose a reason for hiding this comment

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

Done.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 19, 2016

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

bnoordhuis
bnoordhuis approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

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/

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
Copy link
Contributor

ofrobots commented Sep 20, 2016

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

@ofrobots
Copy link
Contributor

ofrobots commented Sep 20, 2016

Thanks, landed as bc1ebd6.

@ofrobots ofrobots closed this Sep 20, 2016
ofrobots pushed a commit that referenced this pull request Sep 20, 2016
@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>
@ofrobots ofrobots added the v6.x label Sep 28, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
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
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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues 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