Skip to content

src: export the ParseEncoding function on Windows - #1596

Closed
ivan wants to merge 1 commit into
nodejs:masterfrom
ivan:parse-encoding-extern
Closed

src: export the ParseEncoding function on Windows#1596
ivan wants to merge 1 commit into
nodejs:masterfrom
ivan:parse-encoding-extern

Conversation

@ivan

@ivanivan commented May 3, 2015

Copy link
Copy Markdown
Contributor

Makes the ParseEncoding symbol visible to addons on Windows. It was already visible on Unices.

I ran into this when writing an addon that used ParseEncoding; the VS2013 build failed with:

(Link target) ->
blake2.obj : error LNK2001: unresolved external symbol "enum node::encoding __cdecl node::ParseEncoding(class v8::Isolate *,class v8::Handle<class v8::Value>,enum node::encoding)" (?ParseEncoding@node@@YA?AW4encoding@1@PEAVIsolate@v8@@V?$Handle@VValue@v8@@@4@W421@@Z) [L:\home\at\NodeProjects\node-blake2\build\blake2.vcxproj]
L:\home\at\NodeProjects\node-blake2\build\Release\blake2.node : fatal error LNK1120: 1 unresolved externals [L:\home\at\NodeProjects\node-blake2\build\blake2.vcxproj]
9 Warning(s)
2 Error(s)

It built fine after adding the NODE_EXPORT.

Similar to an older fixnodejs/node-v0.x-archive#3811

@mscdexmscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 3, 2015
@Fishrock123Fishrock123 added the windows Issues and PRs related to the Windows platform. label May 4, 2015
@Fishrock123

Copy link
Copy Markdown
Contributor

cc @indutny / @bnoordhuis

Comment threadsrc/node.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move v8::Isolate* and friends on a next line at 4 space indent. No need to cascade things ;)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

@indutny

Copy link
Copy Markdown
Member

Generally, LGTM. Is there anything else that needs exporting?

Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.
@ivan

ivan commented May 7, 2015

Copy link
Copy Markdown
ContributorAuthor

Generally, LGTM. Is there anything else that needs exporting?

Not that I know about, in node.h at least.

@indutny

Copy link
Copy Markdown
Member

Ok, @Fishrock123 could you please land this?

@indutny

Copy link
Copy Markdown
Member

@ivan btw, thank you for fixing this! ;)

@Fishrock123

Copy link
Copy Markdown
Contributor

@indutny Yep, will do.

@Fishrock123Fishrock123 mentioned this pull request May 7, 2015
Fishrock123 pushed a commit that referenced this pull request May 7, 2015
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.
PR-URL: #1596
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123

Copy link
Copy Markdown
Contributor

Thanks, landed in c43855c

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.
PR-URL: nodejs#1596
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for freeto 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++.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add NODE_EXTERN to node::Encode

4 participants

@ivan@Fishrock123@indutny@mscdex