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

curl_threads: fix MSVC compiler warning #1717

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MarcelRaad
Member

MarcelRaad commented Aug 1, 2017

Use LongToHandle to convert from long to HANDLE in the Win32
implementation.
This should fix the following warning when compiling with
MSVC 11 (2012) in 64-bit mode visible in #1711:

lib\curl_threads.c(113): warning C4306: 'type cast' : conversion from 'long' to 'HANDLE' of greater size

LongToHandle is available in the February 2003 Platform SDK, so I think we should be safe. Alternatively, INVALID_HANDLE_VALUE could be used (the documented value in the _beginthreadex documentation is -1L, though).

@coveralls

This comment has been minimized.

coveralls commented Aug 1, 2017

Coverage Status

Coverage increased (+0.09%) to 75.355% when pulling 06a57ae on MarcelRaad:msvc11_c4306 into 53d137d on curl:master.

curl_threads: fix MSVC compiler warning
Use LongToHandle to convert from long to HANDLE in the Win32
implementation.
This should fix the following warning when compiling with
MSVC 11 (2012) in 64-bit mode:
lib\curl_threads.c(113): warning C4306:
'type cast' : conversion from 'long' to 'HANDLE' of greater size

Closes #1717

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:msvc11_c4306 branch from 06a57ae to 12b4fb7 Aug 1, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 1, 2017

Coverage Status

Coverage increased (+0.04%) to 75.321% when pulling 12b4fb7 on MarcelRaad:msvc11_c4306 into 821a085 on curl:master.

@bagder

bagder approved these changes Aug 1, 2017

@MarcelRaad MarcelRaad closed this in 0139545 Aug 1, 2017

@MarcelRaad MarcelRaad deleted the MarcelRaad:msvc11_c4306 branch Aug 1, 2017

@jay

This comment has been minimized.

Member

jay commented Aug 1, 2017

I can't reproduce this warning in VS 2010 in a test project at W4 or 2015 in curl project at W4. original mingw only defines LongToHandle if win64 so that will need to be addressed. from what i understand beginthreadex can return 0 or -1 uintptr_t on error (0 but it contradicts because -1 in some versions if startaddress is null). CreateThread can return 0 HANDLE on error. do you get a warning if you drop the L for example if((t == 0) || (t == (curl_thread_t)(uintptr_t)-1)) {

@jay

This comment has been minimized.

Member

jay commented Aug 1, 2017

I tried in VS2010 curl project at W4 and I can reproduce it there, this fixes it:

diff --git a/lib/curl_threads.c b/lib/curl_threads.c
index 1989710..53fed9c 100644
--- a/lib/curl_threads.c
+++ b/lib/curl_threads.c
@@ -110,7 +110,7 @@ curl_thread_t Curl_thread_create(unsigned int (CURL_STDCALL *func) (void *),
 #else
   t = (curl_thread_t)_beginthreadex(NULL, 0, func, arg, 0, NULL);
 #endif
-  if((t == 0) || (t == (curl_thread_t)-1L)) {
+  if((t == 0) || (t == (curl_thread_t)(uintptr_t)-1)) {
 #ifdef _WIN32_WCE
     DWORD gle = GetLastError();
     errno = ((gle == ERROR_ACCESS_DENIED ||
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 1, 2017

Thanks @jay! That's strange as original MinGW cannot even compile for Win64.
Unfortunately older MSVC versions don't have uintptr_t, IIRC it was introduced in VS 2003. Maybe LONG_PTR can be used instead, that would have been my preferred alternative, but I decided for LongToHandle as they were introduced in the same SDK version. I'll check tomorrow.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 2, 2017

@jay I can compile successfully with MinGW and --enable-threaded-resolver --disable-pthreads. It looks like LongToHandle is only defined when not compiling for Win64, so it should be OK. I had to look twice too though, the code in basetsd.h looks like this:

#if defined(_WIN64)
#define __int3264   __int64
#define ADDRESS_TAG_BIT 0x40000000000UI64
#else /*  !_WIN64 */
#define __int3264   __int32
#define ADDRESS_TAG_BIT 0x80000000UL
#define HandleToUlong( h ) ((ULONG)(ULONG_PTR)(h) )
#define HandleToLong( h ) ((LONG)(LONG_PTR) (h) )
#define LongToHandle( h) ((HANDLE)(LONG_PTR) (h))
#define PtrToUlong( p ) ((ULONG)(ULONG_PTR) (p) )
#define PtrToLong( p ) ((LONG)(LONG_PTR) (p) )
#define PtrToUint( p ) ((UINT)(UINT_PTR) (p) )
#define PtrToInt( p ) ((INT)(INT_PTR) (p) )
#define PtrToUshort( p ) ((unsigned short)(ULONG_PTR)(p) )
#define PtrToShort( p ) ((short)(LONG_PTR)(p) )
#define IntToPtr( i )    ((VOID*)(INT_PTR)((int)i))
#define UIntToPtr( ui )  ((VOID*)(UINT_PTR)((unsigned int)ui))
#define LongToPtr( l )   ((VOID*)(LONG_PTR)((long)l))
#define ULongToPtr( ul )  ((VOID*)(ULONG_PTR)((unsigned long)ul))
#endif /* !_WIN64 */
@jay

This comment has been minimized.

Member

jay commented Aug 3, 2017

It looks like LongToHandle is only defined when not compiling for Win64, so it should be OK. I had to look twice too though, the code in basetsd.h looks like this:

You're right, I had misinterpreted that, thanks for following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment