Skip to content

src: export the ParseEncoding function on Windows #1596

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
ivan wants to merge 1 commit into from
Closed

src: export the ParseEncoding function on Windows #1596

ivan wants to merge 1 commit into from

Conversation

@ivan
Copy link
Contributor

@ivan ivan commented May 3, 2015

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 fix nodejs/node-v0.x-archive#3811

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

Fishrock123 commented May 7, 2015

cc @indutny / @bnoordhuis

indutny
indutny reviewed May 7, 2015
View reviewed changes
src/node.h Outdated
Copy link
Member

@indutny indutny May 7, 2015

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
Contributor Author

@ivan ivan May 7, 2015

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
Member

indutny commented May 7, 2015

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

@ivan
src: export the ParseEncoding function on Windows
86342c7 
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.
@ivan
Copy link
Contributor Author

ivan commented May 7, 2015

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

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

@indutny
Copy link
Member

indutny commented May 7, 2015

Ok, @Fishrock123 could you please land this?

@indutny
Copy link
Member

indutny commented May 7, 2015

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

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 7, 2015

@indutny Yep, will do.

@Fishrock123 Fishrock123 mentioned this pull request May 7, 2015
Closed
Fishrock123 pushed a commit that referenced this pull request May 7, 2015
@ivan @Fishrock123
src: export the ParseEncoding function on Windows
c43855c 
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.

PR-URL: #1596
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Fishrock123 commented May 7, 2015

Thanks, landed in c43855c

@Fishrock123 Fishrock123 closed this May 7, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
@ivan @Fishrock123
src: export the ParseEncoding function on Windows
98b1109 
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.

PR-URL: nodejs#1596
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@B020239 B020239 mentioned this pull request Mar 9, 2024
Open
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++. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add NODE_EXTERN to node::Encode

4 participants

@ivan @Fishrock123 @indutny @mscdex