inspector: remove AgentImpl #12576
inspector: remove AgentImpl #12576
Conversation
nodejs-github-bot
added
c++
src/inspector_agent.cc
Outdated
| @@ -44,6 +44,11 @@ std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate, | |||
| return StringBuffer::create(StringView(*buffer, buffer.length())); | |||
| } | |||
|
|
|||
| // Called from the main thread. | |||
| void StartInspectorIoThreadAsyncCallback(uv_async_t* handle) { | |||
| reinterpret_cast<Agent*>(handle->data)->StartIoThread(); | |||
src/inspector_agent.cc
Outdated
|
|
||
| // Header has unique_ptr to some incomplete types - this definition tells | ||
| // the compiler to figure out destruction here, were those types are complete | ||
| Agent::~Agent() { |
There was a problem hiding this comment.
Wouldn't ~Agent() = default; in the header file work?
There was a problem hiding this comment.
No, the compiler is unable to instantiate ~unique_ptr at that point as it does not know if the classes have destructors.
There was a problem hiding this comment.
I think I get it now, perhaps the comment is a little vague. The "incomplete types" refers to Agent's unique_ptr data members, not to a std::unique_ptr<Agent> instance elsewhere, correct?
Vague or not, it should at least be punctuated. :-)
src/inspector_agent.h
Outdated
| static void InspectorConsoleCall( | ||
| const v8::FunctionCallbackInfo<v8::Value>& info); | ||
| static void InspectorWrapConsoleCall( | ||
| const v8::FunctionCallbackInfo<v8::Value>& args); |
There was a problem hiding this comment.
Tiniest of nits: inconsistent parameter names (info vs. args.)
There was a problem hiding this comment.
Fixed, also renamed in the cc file.
src/inspector_agent.cc
Outdated
|
|
||
| // Header has unique_ptr to some incomplete types - this definition tells | ||
| // the compiler to figure out destruction here, were those types are complete | ||
| Agent::~Agent() { |
There was a problem hiding this comment.
I think I get it now, perhaps the comment is a little vague. The "incomplete types" refers to Agent's unique_ptr data members, not to a std::unique_ptr<Agent> instance elsewhere, correct?
Vague or not, it should at least be punctuated. :-)
|
CI: https://ci.nodejs.org/job/node-test-pull-request/7642/ |
|
@eugeneo You missed this though:
|
|
@bnoordhuis I tried rewording the comment in another (unrelated) PR: https://github.com/nodejs/node/pull/12777/files#diff-8d16a44747475ef748bab83e5f9dade2R346 |
AgentImpl was introduce so inspector-agent.h does not leak libuv and
inspector implementation details. Inspector had been reworked since and
new classes provide this isolation and AgentImpl became unnecessary
level of indirection. This change removes that class to streamline the
code.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
inspector - this is a pure refactoring change.