Skip to content
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

src: export the ParseEncoding function on Windows #1596

Closed
wants to merge 1 commit into from
Closed

src: export the ParseEncoding function on Windows #1596

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

cc @indutny / @bnoordhuis

indutny
indutny reviewed May 7, 2015
View reviewed changes
src/node.h
enum encoding ParseEncoding(v8::Isolate* isolate,
v8::Handle<v8::Value> encoding_v,
enum encoding default_encoding = BINARY);
NODE_EXTERN enum encoding ParseEncoding(v8::Isolate* isolate,
Copy link
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
Contributor Author

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

@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 <fedor@indutny.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
Copy link
Contributor

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 <fedor@indutny.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@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