Skip to content

inspector: make Coverity happy#17656

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

inspector: make Coverity happy#17656
eugeneo wants to merge 1 commit intonodejs:masterfrom
eugeneo:better_socket

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Dec 13, 2017 •
edited
Loading

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

inspector: trivial code changes

@nodejs-github-bot nodejs-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 Dec 13, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 13, 2017

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

bnoordhuis
bnoordhuis reviewed Dec 13, 2017
View reviewed changes
src/inspector_socket_server.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Dec 13, 2017

Choose a reason for hiding this comment

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

I did the same in #17649.

Copy link
Contributor Author

@eugeneo eugeneo Dec 13, 2017

Choose a reason for hiding this comment

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

Ok, I'll rebase once your patch lands.

@TimothyGu TimothyGu added the blocked PRs that are blocked by other issues or PRs. label Dec 14, 2017
@maclover7
Copy link
Contributor

maclover7 commented Dec 17, 2017

@eugeneo Are you able to rebase this since #17649 landed?

@TimothyGu TimothyGu removed the blocked PRs that are blocked by other issues or PRs. label Dec 17, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 18, 2017

@maclover7 Did that. Not sure if this will actually fix the Coverity warning...

bnoordhuis
bnoordhuis approved these changes Dec 18, 2017
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 but the commit log needs an update.

@eugeneo eugeneo changed the title inspector: make clang and Coverity happy inspector: make Coverity happy Dec 18, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 18, 2017

@bnoordhuis Done.

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 20, 2017

Another CI run: https://ci.nodejs.org/job/node-test-pull-request/12236/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017
@addaleax
Copy link
Member

addaleax commented Dec 27, 2017

Landed in c0e9cde

addaleax pushed a commit that referenced this pull request Dec 27, 2017
@addaleax
inspector: make Coverity happy
c0e9cde 
PR-URL: #17656
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax closed this Dec 27, 2017
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@MylesBorins
inspector: make Coverity happy
b1113a5 
PR-URL: #17656
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@MylesBorins
inspector: make Coverity happy
fa371e8 
PR-URL: #17656
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@MylesBorins
inspector: make Coverity happy
8d4fca3 
PR-URL: #17656
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@gibfahn
Copy link
Member

gibfahn commented Jan 24, 2018

This doesn't seem to be needed on LTS branches

@eugeneo eugeneo deleted the better_socket branch April 26, 2018 17:34
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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@eugeneo @maclover7 @addaleax @gibfahn @bnoordhuis @jasnell @TimothyGu @cjihrig @nodejs-github-bot