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

added native windows threading support #101

Closed
wants to merge 9 commits into from
Closed

added native windows threading support #101

wants to merge 9 commits into from

Conversation

chacham15
Copy link

I wanted to compile libetpan under mingw and not use the mingw-pthread library so I implemented all the changes necessary. The only thing that I am a bit uncertain about (although in practice it seems to work fine) is a result of the lack of a static initializer in windows of a mutex. Therefore, I added some initialization stuff in mailstream_ssl.c at mailstream_ssl_init. The code itself will run initialization only in one thread. The only problem is that if two threads run it simultaneously, the thread that did not do the initialization might continue on to use the thread primitives while they are being initalized by the other thread. Naturally, this is only a problem for this compilation of windows, but so far I see no problem. If you have any ideas of how to fix that minor problem I would be grateful. In any case, the changes I have made here do seem to work for mingw with native windows threads if you would like to import back to the main branch.

@dinhvh
Copy link
Owner

dinhvh commented Jan 12, 2014

  • The pull request is failing to build.
  • The diff is very hard to read, probably because you changed LF to CR LF.

Could you clean up so it's easy to read the diff?

Thanks!

@chacham15
Copy link
Author

Ok, I dos2unixed the files so the diff should be more readable. I also fixed the few stupid mistakes causing the build issues on the other platform.

@@ -183,6 +80,167 @@ enum {
SEMKIND_INTERNAL
};

#if (defined(LIBETPAN_REENTRANT) && defined(HAVE_PTHREAD_H) && !defined(IGNORE_PTHREAD_H)) || !defined(LIBETPAN_REENTRANT)

static int mailsem_internal_init(struct mailsem_internal * s,
Copy link
Owner

Choose a reason for hiding this comment

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

Could you unindent the content of #ifdef?

@chacham15
Copy link
Author

Is that better? The spacing comments confused me a bit, I think that the issue was that I was using hard-tabs so I changed them to soft tabs.

@dinhvh
Copy link
Owner

dinhvh commented Jan 13, 2014

Going through the diff, there's still lots of indentation changes.

@dinhvh
Copy link
Owner

dinhvh commented Jan 13, 2014

Could you go through "Files Changed" section and make sure there's no odd diff.

@chacham15
Copy link
Author

Could you give me a general overview of how you would like the indentation to be?

E.g. use soft tabs not hard tabs, do not indent sub #defines, etc.

@dinhvh
Copy link
Owner

dinhvh commented Jan 13, 2014

  • soft tabs, new spaces.
  • indents with blocks.
  • usually don't indent on #ifdef

Usually, those rules are not too strict.

The things to fix are the changes in spacing and indentation.
I'll highlight some of them.

r = pthread_mutex_init(&s->lock, NULL);
if (r != 0)
goto err;
Copy link
Owner

Choose a reason for hiding this comment

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

Here.

@chacham15
Copy link
Author

How is that? (For some reason the diffs dont show the contents of the file. Is there a delay on the diffs?)

@dinhvh
Copy link
Owner

dinhvh commented Jan 13, 2014

@chacham15
I think I'll merge manually your change.
Thanks.

@dinhvh
Copy link
Owner

dinhvh commented Jan 14, 2014

btw, why did you introduce IGNORE_PTHREAD_H?

@chacham15
Copy link
Author

Because it is possible to have (and use) pthreads under mingw. Therefore, I added that option to allow one to explicitly build with only windows threads.

@dinhvh
Copy link
Owner

dinhvh commented Jan 14, 2014

IGNORE_PTHREAD_H is never defined. How do you set it?

@dinhvh
Copy link
Owner

dinhvh commented Jan 14, 2014

I merge most of it in e67180b.

@chacham15
Copy link
Author

apologies, I added it because I knew that it would be useful, not because I personally would need it at this time. I went back and added the proper definition to configure.ac.

@dinhvh
Copy link
Owner

dinhvh commented Jan 14, 2014

I'm closing this pull request now since I merged manually.

@dinhvh dinhvh closed this Jan 14, 2014
@dinhvh
Copy link
Owner

dinhvh commented Jan 14, 2014

However, could you check if it builds properly for you on Windows?
Thanks.

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.

3 participants