Re: fusion: Use proper function pointer

[James Hawkins]

On Sat, Aug 2, 2008 at 4:09 PM, Andrew Talbot
 wrote:
> Changelog:
>    fusion: Use proper function pointer.
>
> diff --git a/dlls/fusion/fusion.c b/dlls/fusion/fusion.c
> index ac01cf4..637346c 100644
> --- a/dlls/fusion/fusion.c
> +++ b/dlls/fusion/fusion.c
> @@ -32,6 +32,9 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(fusion);
>
> +typedef HRESULT (WINAPI *PFNGETCORVERSION)(LPWSTR pbuffer, DWORD cchBuffer,
> -
>                                       DWORD *dwLength);
> -
>
> /******************************************************************
> - ClearDownloadCache   (FUSION.@)
>
> */
> @@ -89,20 +92,18 @@ HRESULT WINAPI GetAssemblyIdentityFromFile(LPCWSTR pwzFilePath, REFIID riid,
>     return E_NOTIMPL;
>  }
>
> -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.

fusion: Use proper function pointer

[Andrew Talbot]

James Hawkins wrote:
> On Sat, Aug 2, 2008 at 4:09 PM, Andrew Talbot
>  wrote:
> > Changelog:
> >    fusion: Use proper function pointer.
> >
> > diff --git a/dlls/fusion/fusion.c b/dlls/fusion/fusion.c
> > index ac01cf4..637346c 100644
> > --- a/dlls/fusion/fusion.c
> > +++ b/dlls/fusion/fusion.c
> > @@ -32,6 +32,9 @@
> >
> > WINE_DEFAULT_DEBUG_CHANNEL(fusion);
> >
> > +typedef HRESULT (WINAPI *PFNGETCORVERSION)(LPWSTR pbuffer, DWORD
> > cchBuffer,
> > -
> >                                       DWORD *dwLength);
> > -
> >
> > /******************************************************************
> > - ClearDownloadCache   (FUSION.@)
> >
> > */
> > @@ -89,20 +92,18 @@ HRESULT WINAPI GetAssemblyIdentityFromFile(LPCWSTR
> > pwzFilePath, REFIID riid,
> >     return E_NOTIMPL;
> >  }
> >
> > -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.

Hi James,

I think the part of my patch you have copied excludes the crucial bit (please excuse the line-wrapping).

-    pGetCORVersion = (void *)GetProcAddress(hmscoree, "GetCORVersion");
+    pGetCORVersion = (PFNGETCORVERSION)GetProcAddress(hmscoree, "GetCORVersion");

The thing that I would argue is wrong is that we are using an object pointer

(void*)

in place of a function pointer

<return type> (*)(<args>)

Granted there are probably around two thousand other instances like this in the dlls, and some may argue that it works, so leave it. On the other hand, there are also quite a lot of type definitions for *PFN...s and several MAKE_FUNCPTR() macros around, too.

I seem to remember that someone tried to compile Wine with Visual Studio or suchlike, recently, and had to suppress warnings about using object pointers instead of function pointers (amongst other things).

Anyway, I just fly the odd flag in good faith, occasionally, and see whether it gets saluted or shot down. :)

Thanks and kind regards,

fusion: Use proper function pointer

[Michael Karcher]

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