Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Dev/win32 minor fixes #1240

Closed
wants to merge 3 commits into from
Closed

Dev/win32 minor fixes #1240

wants to merge 3 commits into from

Conversation

zerhacken
Copy link
Contributor

No description provided.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit zerhacken/libuv@f7cdc53aeffb465d266f6db8b0aabe922f7e7152 has the following error(s):

  • First line of commit message must be no longer than 50 characters

Commit zerhacken/libuv@28db63c2717b60036daad036111378c7823675fb has the following error(s):

  • First line of commit message must be no longer than 50 characters

The following commiters were not found in the CLA:

  • Timothy J Fontaine
  • Rasmus Pedersen

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@@ -97,6 +97,7 @@ TEST_IMPL(getaddrinfo_concurrent) {
callback_counts[i] = 0;

data = (int*)malloc(sizeof(int));
ASSERT(data != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please adjust style here? Also please make sure to use spaces and not tabs everywhere.

@indutny
Copy link
Contributor

indutny commented Apr 16, 2014

Generally looks good, but needs rebasing and fixing nits (+ CLA signature is required, perhaps)

@zerhacken
Copy link
Contributor Author

Sorry, will fix.

@zerhacken
Copy link
Contributor Author

Signed the CLA and fixed up commits.

@@ -86,7 +86,8 @@ static void saturate_threadpool(void) {
* the thread pool is saturated. As with any timing dependent test,
* this is obviously not ideal.
*/
if (uv_cond_timedwait(&signal_cond, &signal_mutex, 350 * 1e6)) {
if (uv_cond_timedwait(&signal_cond, &signal_mutex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put if(uv_cond_timed_wait(&signal_cond &signal_mutx and (uint64_t)... on a separate lines

@indutny
Copy link
Contributor

indutny commented Apr 16, 2014

Generally LGTM, @saghul mind taking a look.

@zerhacken
Copy link
Contributor Author

Hope this is better.

@saghul
Copy link
Contributor

saghul commented Apr 17, 2014

LGTM, but what is tj's commit doing there? Is it not pushed already?

@zerhacken
Copy link
Contributor Author

Ain't sure why tj's commit is there, I could recreate my stuff in a new branch if necessary?

@saghul
Copy link
Contributor

saghul commented Apr 17, 2014

No need, just rebase your patches on top of master and force push. That should do :-)

@txdv
Copy link
Contributor

txdv commented Apr 17, 2014

If you rebase properly, you don't need to force push?

@saghul
Copy link
Contributor

saghul commented Apr 17, 2014

@txdv AFAIK, you do.

@zerhacken
Copy link
Contributor Author

@saghul rebased and pushed but still seeing tj's commit a42e55f

@saghul
Copy link
Contributor

saghul commented Apr 17, 2014

No worries, I'll squeash/rebase when landing. LGTM.

@saghul
Copy link
Contributor

saghul commented Apr 17, 2014

Thanks @zerhacken, landed in cd6e74d

@saghul saghul closed this Apr 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants