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.

Sign up for GitHub

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

Jump to bottom

tools: roll inspector_protocol to f67ec5 #26303

Merged
merged 1 commit into from
Mar 4, 2019
Merged

tools: roll inspector_protocol to f67ec5 #26303

merged 1 commit into from
Mar 4, 2019

Conversation

pavelfeldman
Copy link
Contributor

Fixes: #25808

@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 Feb 25, 2019
@pavelfeldman pavelfeldman mentioned this pull request Feb 25, 2019
Closed
addaleax
addaleax approved these changes Feb 25, 2019
View reviewed changes
src/inspector/node_string.cc
return message;
}

ProtocolMessage binaryToMessage(std::vector<uint8_t> message) {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

src/inspector/node_string.h Outdated
class Binary {
public:
const uint8_t* data() const {
DCHECK(false);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@targos
Copy link
Member

targos commented Feb 25, 2019

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
Copy link
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 pavelfeldman changed the title Roll tools/inspector_protocol to f67ec5180f476830e839226b5ca948e43070… tools: roll inspector_protocol to f67ec5180f476830e839226b5ca948e43070… Feb 25, 2019
@pavelfeldman pavelfeldman changed the title tools: roll inspector_protocol to f67ec5180f476830e839226b5ca948e43070… tools: roll inspector_protocol to f67ec5 Feb 25, 2019
refack
refack approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

RSLGTM

@refack
Copy link
Contributor

refack commented Feb 26, 2019

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

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

@addaleax
Copy link
Member

addaleax commented Mar 2, 2019

@pavelfeldman What’s the status of this PR?

@targos
Copy link
Member

targos commented Mar 4, 2019

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

@refack refack self-assigned this Mar 4, 2019
@refack
Copy link
Contributor

refack commented Mar 4, 2019

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

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

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

PR-URL: nodejs#26303
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
@pavelfeldman @BethGriggs
tools: roll inspector_protocol to f67ec5
4819296 
Fixes: #25808

PR-URL: #26303
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@BethGriggs 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

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
@pavelfeldman @targos @refack @addaleax @nodejs-github-bot