Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

src: export Agent's constructor and destructor #3046

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.

Sign up for GitHub

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

Jump to bottom
Closed
zcbenz wants to merge 1 commit into from
Closed

src: export Agent's constructor and destructor #3046

zcbenz wants to merge 1 commit into from

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

trevnorris commented Sep 25, 2015

LGTM

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 25, 2015

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

bnoordhuis commented Sep 25, 2015

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

@zcbenz zcbenz mentioned this pull request Sep 28, 2015
Closed
@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

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zcbenz @rvagg @indutny @trevnorris @bnoordhuis @brendanashworth
Morty Proxy This is a proxified and sanitized view of the page, visit original site.