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

test: fix missing unistd.h on windows #3532

Closed
wants to merge 1 commit into from
Closed

Conversation

john-yan
Copy link

PL-URL: #3531

@Fishrock123 Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Oct 26, 2015
@Fishrock123
Copy link
Contributor

cc @bnoordhuis probably

@john-yan
Copy link
Author

@bnoordhuis Would you please also land it on v4.x? Thanks.

@@ -13,7 +20,8 @@ struct async_req {

void DoAsync(uv_work_t* r) {
async_req* req = reinterpret_cast<async_req*>(r->data);
sleep(1); // Simulate CPU intensive process...
// Simulate CPU intensive process...
std::this_thread::sleep_for(std::chrono::seconds(1));
Copy link
Member

Choose a reason for hiding this comment

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

This won't work on OS X due to lack of library support. Can I suggest something like this?

#ifdef _WIN32
  Sleep(1000);
#else
  sleep(1);
#endif

Copy link
Author

Choose a reason for hiding this comment

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

I am happy to use that.

Copy link
Author

Choose a reason for hiding this comment

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

But since the latest v8 is also using c++11, I think OS X might be able to get around c++11 problems by using new compilers and libraries.

Copy link
Member

Choose a reason for hiding this comment

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

There are two aspects to C++11 support: compiler support (okay on OS X) and libc++ support (which is lacking.)

Copy link
Author

Choose a reason for hiding this comment

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

I noticed the default libc++ library on OS X is outdated and doesn't support c++11. I wonder how OS X user compile the latest v8, which is part of nodejs and using c++11? they should have a solution to that. (I think they download the lib from somewhere)

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis
Copy link
Member

bnoordhuis pushed a commit that referenced this pull request Oct 28, 2015
PR-URL: #3532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Thanks, landed in 4139f2a.

@john-yan
Copy link
Author

hello @jasnell , would you please help land it on v4.x? thanks.

rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
PR-URL: nodejs#3532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Oct 29, 2015
jasnell pushed a commit that referenced this pull request Oct 29, 2015
PR-URL: #3532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell pushed a commit that referenced this pull request Oct 29, 2015
PR-URL: #3532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
john-yan pushed a commit to ibmruntimes/node that referenced this pull request Oct 29, 2015
PR-URL: nodejs/node#3532
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Oct 29, 2015

Landed in v4.x-staging in 2fc13e5

Sign up for free to 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++. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants