Skip to content

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Sep 24, 2015

Since node::Environment includes node::debugger::Agent as a member data, and the constructor and destructor of node::Environment are inlined, we have to export Agent's constructor and destructor, otherwise when Node is built as shared library we will get following error:

electron_lib.lib(electron_lib.node_main.obj) : error LNK2019: unresolved external symbol "public: __cdecl node::debugger::Agent::~Agent(void)" (??1Agent@debugger@node@@QEAA@XZ) referenced in function "private: __cdecl node::Environment::~Environment(void)" (??1Environment@node@@AEAA@XZ)
electron_lib.lib(electron_lib.atom_renderer_client.obj) : error LNK2001: unresolved external symbol "public: __cdecl node::debugger::Agent::~Agent(void)" (??1Agent@debugger@node@@QEAA@XZ)
electron_lib.lib(electron_lib.atom_renderer_client.obj) : error LNK2019: unresolved external symbol "public: __cdecl node::debugger::Agent::Agent(class node::Environment *)" (??0Agent@debugger@node@@QEAA@PEAVEnvironment@2@@Z) referenced in function "private: __cdecl node::Environment::Environment(class v8::Local<class v8::Context>,struct uv_loop_s *)" (??0Environment@node@@AEAA@V?$Local@VContext@v8@@@v8@@PEAUuv_loop_s@@@Z)

@brendanashworth brendanashworth added c++ Issues and PRs that require attention from people who are familiar with C++. debugger labels Sep 24, 2015
@rvagg
Copy link
Member

rvagg commented Sep 24, 2015

lgtm

@zcbenz is this the only blocker for you using it as a shared library? was it working properly for electron prior to v4?

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 24, 2015

@rvagg Yeah it is the only blocker, before debugger_agent became a member of node::Environment things worked fine.

@rvagg
Copy link
Member

rvagg commented Sep 25, 2015

@trevnorris, @bnoordhuis, @indutny any of you care to give this a quick look?

@rvagg
Copy link
Member

rvagg commented Sep 25, 2015

also #3058 while you're at it

@indutny
Copy link
Member

indutny commented Sep 25, 2015

LGTM

1 similar comment
@trevnorris
Copy link
Contributor

LGTM

@bnoordhuis
Copy link
Member

Wouldn't it make more sense to move the Environment constructor and destructor to src/env.cc? This is leaking an implementation detail.

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 25, 2015

@bnoordhuis Both ways are good to me, I can create a new PR if you prefer that way.

@bnoordhuis
Copy link
Member

Moving the constructor and destructor would have my preference. It's more churn initially but more maintainable long-term.

@rvagg
Copy link
Member

rvagg commented Sep 29, 2015

good call @bnoordhuis, closing in favour of #3098

@rvagg rvagg closed this Sep 29, 2015
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++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants