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.

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

build: fix build error for clang-cl #67

Merged
merged 1 commit into from
May 1, 2018

Conversation

hashseed
Copy link
Member

Clang complains that the declaration and definition differs (extern vs extern "C"). This fixes the issue.

@hashseed
Copy link
Member Author

@bnoordhuis @mi-ac

@hashseed
Copy link
Member Author

I was able to confirm that this patch works and fixes the Clang build on Windows.

@@ -32,7 +32,7 @@

namespace node {

extern HANDLE NodeCounterProvider;
EXTERN_C HANDLE NodeCounterProvider;

Choose a reason for hiding this comment

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

It should still be extern here, otherwise including the header in two source files leads to it getting defined twice.

(I guess in practice it ends up in the Windows equivalent of .common and the linker merges them but it's best not to depend on that.)

@hashseed
Copy link
Member Author

@bnoordhuis do you like the current version better?

Copy link

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Yep, LGTM.

@hashseed hashseed merged commit 8a3c087 into v8:vee-eight-lkgr May 1, 2018
@hashseed hashseed deleted the fixclangcl branch July 13, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants