inspector: remove AgentImpl #12576
inspector: remove AgentImpl #12576
Conversation
nodejs-github-bot
added
c++
src/inspector_agent.cc
Outdated
src/inspector_agent.cc
Outdated
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
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
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.