FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: roll inspector_protocol to f67ec5 by pavelfeldman · Pull Request #26303 · nodejs/node · GitHub

/ node Public

tools: roll inspector_protocol to f67ec5 - #26303

Merged
refack merged 1 commit into
nodejs:masterfrom
pavelfeldman:inspector_protocol
Mar 4, 2019
ConversationCommits (1)ChecksFiles changed
Merged

tools: roll inspector_protocol to f67ec5#26303
refack merged 1 commit into
nodejs:masterfrom
pavelfeldman:inspector_protocol

Conversation

pavelfeldman commented Feb 25, 2019

Copy link
Copy Markdown
Contributor

Fixes: #25808

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 Feb 25, 2019
pavelfeldman mentioned this pull request Feb 25, 2019
Closed
addaleax approved these changes Feb 25, 2019
View reviewed changes
Comment thread src/inspector/node_string.cc Outdated

addaleax Feb 25, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should probably be

Suggested change
ProtocolMessage binaryToMessage(std::vector<uint8_t> message) {
ProtocolMessage binaryToMessage(const std::vector<uint8_t>& message) {

?

pavelfeldman Feb 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not really, we are std::move-ing binary messages around. It is likely that we switch to using std::unique_ptr<std::vector> for clarity at some point.

Comment thread src/inspector/node_string.h Outdated

addaleax Feb 25, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it make sense to UNREACHABLE();? You can drop the return statements in that case.

pavelfeldman Feb 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, perfect, that's what it is in v8/chromium, I was not sure if node prefers that as well.

pavelfeldman Feb 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correction, it was UNIMPLEMENTED in v8/chromium. Anyhow, using UNREACHABLE now

targos commented Feb 25, 2019

Copy link
Copy Markdown
Member

Is there a guide/documentation on how to update this dependency? This PR removes more lines than it adds while my try here adds 7k.

pavelfeldman commented Feb 25, 2019

Copy link
Copy Markdown
Contributor Author

Is there a guide/documentation on how to update this dependency?

we have scripts doing that in the v8 / chromium lands, let us add another one for Node.

pavelfeldman changed the title Roll tools/inspector_protocol to f67ec5180f476830e839226b5ca948e43070… tools: roll inspector_protocol to f67ec5180f476830e839226b5ca948e43070… Feb 25, 2019
pavelfeldman changed the title tools: roll inspector_protocol to f67ec5180f476830e839226b5ca948e43070… tools: roll inspector_protocol to f67ec5 Feb 25, 2019
refack approved these changes Feb 26, 2019
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

refack commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

@pavelfeldman, thank you for the bump.
Could you have forgotten to add renamed files?

From Travis:

make[1]: *** No rule to make target `../tools/inspector_protocol/code_generator.py', needed by `26acdf201a53d5709754b88c308e871df22184f9.intermediate'.  Stop.

pavelfeldman commented Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Turns out I lost all the added third party files while shuffling them between the repos :/ ...

addaleax commented Mar 2, 2019

Copy link
Copy Markdown
Member

@pavelfeldman What’s the status of this PR?

targos commented Mar 4, 2019

Copy link
Copy Markdown
Member

I think it's ready? I confirm this PR fixes the canary builds.

refack self-assigned this Mar 4, 2019

refack commented Mar 4, 2019

Copy link
Copy Markdown
Contributor

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

cclauss mentioned this pull request Mar 4, 2019
Closed
63 tasks
tools: roll inspector_protocol to f67ec5
d775d74 
Fixes: nodejs#25808

PR-URL: nodejs#26303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack force-pushed the inspector_protocol branch from c5a8f91 to d775d74 Compare March 4, 2019 16:48
refack merged commit d775d74 into nodejs:master Mar 4, 2019
refack removed their assignment Mar 11, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
tools: roll inspector_protocol to f67ec5
d841a89 
Fixes: nodejs#25808

PR-URL: nodejs#26303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR mentioned this pull request Mar 13, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
tools: roll inspector_protocol to f67ec5
4819296 
Fixes: #25808

PR-URL: #26303
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
This was referenced May 29, 2019
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
+1 more reviewer
refack refack 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.

5 participants

Back | FazBrowse Home | New Git URL