[
https://issues.apache.org/jira/browse/THRIFT-1031?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13113043#comment-13113043
]
alexandre parenteau edited comment on THRIFT-1031 at 9/22/11 11:59 PM:
-----------------------------------------------------------------------
Hi, thanks all for making this happen!
I had a chance finally to compile/run it (using the Calculator), and here are
my comments/questions for @James:
1- overall definitively this looks good: I guess there is no WIN64 target,
because there is no pthread for WIN64?
2- Cosmetic: something needs to define "#define HAVE_GETTIMEOFDAY"
(windows/config.h?)
3- Cosmetic: #define __BYTE_ORDER: this is not necessary anymore, another patch
of mine previously fixed the auto detection on win32 using boost. The README
may also reflect that (double should be fine).
4- Cosmetic: typedef __kernel_size_t size_t, typedef __kernel_ssize_t ssize_t:
I found this easier version: typedef ptrdiff_t ssize_t; (don't think size_t
needs to be defined AFAIK)
5- Cosmetic: #define NOMINMAX (before Windows.h, because std::min is used in
thrift)
6- Cosmetic warning: #pragma warning(disable: 4996) and #pragma
warning(disable: 4250) could be nice ("POSIX name for this item is deprecated",
"inherits via dominance")
7- config.h: here is my biggest concern: you decided (honorably!) to re-use
config.h (and define HAVE_CONFIG_H): I find it in practice annoying because it
assumes "config.h" is at the same level of "Thrift.h". This forces in turn to
add <thrift folder>/windows to the include path. What happens is when mixing
with other libraries who also use config.h, it may take precedence over
thrift's config.h. Ideally, I would prefer if *not* using HAVE_CONFIG_H, and do
something like this instead:
#ifdef _WIN32
#include "windows/config.h"
$endif
When included from Thrift.h, this guarantees to pick the right config.h (and
not Python's one for example).
Now what happens for the source code .cpp which does not include Thrift.h? Well
since you are using already force-compilation, you could add windows/config.h
to the list for forced headers. I found this to work in practice.
Again, thanks a lot for the patch!
was (Author: aubonbeurre):
Hi, thanks all for making this happen!
I had a chance finally to compile/run it (using the Calculator), and here are
my comments/questions for @James:
1- overall definitively this looks good: I guess there is no WIN64 target,
because there is no pthread for WIN64?
2- Cosmetic: something needs to define "#define HAVE_GETTIMEOFDAY"
(windows/config.h?)
3- Cosmetic: #define __BYTE_ORDER: this is not necessary anymore, another patch
of mine previously fixed the auto detection on win32 using boost. The README
may also reflect that (double should be fine).
4- Cosmetic: typedef __kernel_size_t size_t, typedef __kernel_ssize_t ssize_t:
I found this easier version: typedef ptrdiff_t ssize_t; (don't think size_t
needs to be defined AFAIK)
5- Cosmetic: #define NOMINMAX (before Windows.h, because std::min is used in
thrift)
6- Cosmetic warning: #pragma warning(disable: 4996) and #pragma
warning(disable: 4250) could be nice ("POSIX name for this item is deprecated",
"inherits via dominance")
7- config.h: here is my biggest concern: you decided (honorably!) to re-use
config.h (and define HAVE_CONFIG_H): I find it in practice annoying because it
assumes "config.h" is at the same level of "Thrift.h". This forces in turn to
add <thrift folder>/windows to the include path. What happens is when mixing
with other libraries who also use config.h, it may take precedence over
thrift's config.h. Ideally, I would prefer if *not* using HAVE_CONFIG_H, and do
something like this instead:
#ifdef _WIN32
#include "windows/config.h"
$endif
When included from Thrift.h, this guarantees to pick the right config.h (and
not Python's one for example).
Now what happens for the source code .cpp? Well since you are using already
force-compilation, you could add windows/config.h to the list for forced
headers.
Again, thanks a lot for the patch!
> Patch to compile Thrift for vc++ 9.0 and 10.0
> ---------------------------------------------
>
> Key: THRIFT-1031
> URL: https://issues.apache.org/jira/browse/THRIFT-1031
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Environment: Windows XP 32bit, vc++ 9.0, 10.0
> Reporter: James Dickson
> Assignee: Roger Meier
> Priority: Trivial
> Fix For: 0.8
>
> Attachments: thrift_msvc.patch, thrift_msvc_v0_1.patch,
> thrift_msvc_v0_2.patch, thrift_msvc_v0_3.patch, thrift_msvc_v0_4.patch,
> thrift_msvc_v0_5.patch, thrift_msvc_v0_6.patch
>
>
> At our company we need clients running on Windows being able to connect to
> our linux servers running hypertable. The attached patch enables the parts
> needed by Hypertable to be compiled on Windows using either the VC++ 9.0 or
> 10.0 compilers.
> Having read previous posts about ports using boost::asio we found these to be
> too intrusive for our needs. This version uses pthreads_win32 and winsock2
> and is as designed to be as un-intrusive as is possible to the original unix
> code base. It is mostly #defines between unix sockets and winsock2 sockets.
> We also tried to follow the folder structuring of the C# runtime that has
> visual studio solutions to be consistent.
> More details are in the README as not all the functionality of the original
> unix code base is available to windows users. We will add the missing
> functionality, we just wanted to share what we had as for a Windows based
> client for us it is sufficient.
> The patch is based on the latest revision in SVN, we would love feedback and
> any code reviews. If there is any possibility of this being added to the main
> trunk then that would be much appreciated, however we don't expect that.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
