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.

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

src: reuse std::make_unique in inspector_agent.cc #24132

Closed
wants to merge 1 commit into from

Conversation

alyssaq
Copy link
Contributor

@alyssaq alyssaq commented Nov 6, 2018

Ref: 283a967

@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 Nov 6, 2018
@addaleax addaleax 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

Trott commented Nov 10, 2018

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]>
@codebytere codebytere mentioned this pull request Jan 4, 2019
Sign up for free to 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-learn Issues related to the Code-and-Learn events and PRs submitted during the events. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants