Am Samstag, den 02.08.2008, 17:51 -0500 schrieb James Hawkins:
-static HRESULT (WINAPI *pGetCORVersion)(LPWSTR pbuffer, DWORD cchBuffer,
DWORD *dwLength);There's nothing wrong with this. We use this type of function pointer all over the code base.
You don't quote the interesting part:
- pGetCORVersion = (void *)GetProcAddress(hmscoree, "GetCORVersion");
- pGetCORVersion = (PFNGETCORVERSION)GetProcAddress(hmscoree, "GetCORVersion");
This *is* wrong, according to the C standard. You may not cast a function pointer to a data pointer (like void*). There might be precedence in Wine to ignore this rule, but the patch seems appropriate at this point, as GetProcAddress does, in fact, return a pointer-to-function (FARPROC, which is typedefed in windef.h to a function pointer), which gets assigned to a function pointer. The (void*) cast is an IMHO misguided attempt to suppress the warning of casting between to function pointers of different types. As there is no void* like generic pointer for function pointers, the only sensible way to suppress the warning without causing undefined behaviour really is casting the result of GetProcAddress to the type of the destination pointer. As the casts to function pointer types look utterly incomprehensible in C, it seems correct to introduce a typedef, like the patch does.
Another point of the patch is that is makes a static global variable a local function variable. I don't take a position on that change, although on the first glance it seems reasonable, as the global variable is initialized on each entry into get_corversion. If the address of GetCORVersion should be cached between invocations, the GetProcAddress and LoadLibraryA calls should probably be guarded by pGetCORVersion != NULL.
Regards, Michael Karcher