Skip to content

Conversation

@alyssaq
Copy link
Contributor

Ref: 283a967

@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 Nov 6, 2018
@addaleaxaddaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2018
@Trott
Copy link
Member

Trott commented Nov 8, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 10, 2018
Ref: nodejs@283a967 PR-URL: nodejs#24132 Refs: nodejs@283a967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
@Trott
Copy link
Member

Landed in 08bd823.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@luto
Copy link

luto commented Dec 2, 2018

@alyssaq The rest of nodejs uses base::make_uniqe, since the std:: version is not yet available in all distributions (CentOS 7 doesn't have it for example).

../src/inspector_agent.cc: In member function 'int node::inspector::NodeInspectorClient::connectFrontend(std::unique_ptr<node::inspector::InspectorSessionDelegate>, bool)': ../src/inspector_agent.cc:479:9: error: 'make_unique' is not a member of 'std' std::make_unique<ChannelImpl>(env_, client_, getWorkerManager(), ^ ../src/inspector_agent.cc:479:37: error: expected primary-expression before '>' token std::make_unique<ChannelImpl>(env_, client_, getWorkerManager(), ^ ../src/inspector_agent.cc:479:45: warning: left operand of comma operator has no effect [-Wunused-value] std::make_unique<ChannelImpl>(env_, client_, getWorkerManager(), ^ ../src/inspector_agent.cc:479:71: warning: right operand of comma operator has no effect [-Wunused-value] std::make_unique<ChannelImpl>(env_, client_, getWorkerManager(), ^ make[1]: *** [/builds/uberspace/packages/nodejs/node/out/Release/obj.target/node_lib/src/inspector_agent.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [node] Error 2 

Is it possible to replace the function like so? I am not a c++ coder myself :)

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index e0ee24a7f0..5c8020f25d 100644 --- a/src/inspector_agent.cc+++ b/src/inspector_agent.cc@@ -12,6 +12,7 @@ #include "node_url.h" #include "v8-inspector.h" #include "v8-platform.h" +#include "src/base/template-utils.h" #include "libplatform/libplatform.h" @@ -476,7 +477,7 @@ class NodeInspectorClient : public V8InspectorClient{events_dispatched_ = true; int session_id = next_session_id_++; channels_[session_id] = - std::make_unique<ChannelImpl>(env_, client_, getWorkerManager(),+ base::make_unique<ChannelImpl>(env_, client_, getWorkerManager(), std::move(delegate), prevent_shutdown); return session_id}

@joyeecheung
Copy link
Member

joyeecheung commented Dec 2, 2018

@ludo I don't think we are able to include headers under deps/v8/src? I think usually when we run into this we just have to roll our own versions of those things...

@nodejs/build I am curious why this was not uncovered from compiler selections in the CI? (I think we ran into something similar once before that was fixed by @addaleax ?)

@joyeecheung
Copy link
Member

joyeecheung commented Dec 2, 2018

@luto The current lowest GCC version we support is 4.9.4 (which is covered on Ubuntu and RHEL in our CI), the default on CentOS 7 is 4.8.x. Can you try upgrading to a newer version of devtoolset if you are on CentOS? I am afraid we will probably leave it as-is since this is within our GCC support and is what we have moved on to - not sure if we are willing to accept a patch to roll our own version of std::make_unique, I'll defer that to @nodejs/build

codebytere pushed a commit that referenced this pull request Dec 14, 2018
Ref: 283a967 PR-URL: #24132 Refs: 283a967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Ref: 283a967 PR-URL: #24132 Refs: 283a967 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]>
@codebyterecodebytere mentioned this pull request Jan 4, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@alyssaq@Trott@luto@joyeecheung@eugeneo@addaleax@cjihrig@nodejs-github-bot