Re: [v2 PATCH 2/3] odbccp32: Implement SQLRemoveTranslator/W

[Huw Davies]

On Wed, Mar 22, 2017 at 10:06:18PM +0000, Alistair Leslie-Hughes wrote:
> diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
> index 7b2f020..d95a110 100644
> --- a/dlls/odbccp32/odbccp32.c
> +++ b/dlls/odbccp32/odbccp32.c
> @@ -1229,20 +1229,75 @@ BOOL WINAPI SQLRemoveDSNFromIni(LPCSTR lpszDSN)
>      return FALSE;
>  }
>
> -BOOL WINAPI SQLRemoveTranslatorW(LPCWSTR lpszTranslator, LPDWORD lpdwUsageCount)
> +BOOL WINAPI SQLRemoveTranslatorW(const WCHAR *translator, DWORD *usage_count)
>  {
> - HKEY hkey;
> - DWORD usagecount = 1;
> - clear_errors();
> - FIXME("%s %p\n", debugstr_w(lpszTranslator), lpdwUsageCount);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return FALSE;
> - TRACE("%s %p\n", debugstr_w(translator), usage_count);
> -
> - if (RegOpenKeyW(HKEY_LOCAL_MACHINE, odbcini, &hkey) == ERROR_SUCCESS)
> - {
> -
>    HKEY hkeydriver;
> -
> -
>    if (RegOpenKeyW(hkey, translator, &hkeydriver) == ERROR_SUCCESS)
> -
>    {
> -
>        DWORD size, type;
> -
>        DWORD count;
> -
> -
>        size = sizeof(usagecount);
> -
>        RegGetValueA(hkeydriver, NULL, "UsageCount", RRF_RT_DWORD, &type, &usagecount, &size);
> -
>        TRACE("Usage count %d\n", count);

I think you mean usagecount here.  It makes me wonder whether you
actually need two variables to represent the same thing.

Huw.

[v2 PATCH 2/3] odbccp32: Implement SQLRemoveTranslator/W

[Hugh McMaster]

On Wed, Mar 22, 2017 at 10:06:18PM +0000, Alistair Leslie-Hughes wrote:
> diff --git a/dlls/odbccp32/odbccp32.c b/dlls/odbccp32/odbccp32.c
> index 7b2f020..d95a110 100644
> --- a/dlls/odbccp32/odbccp32.c
> +++ b/dlls/odbccp32/odbccp32.c
> @@ -1229,20 +1229,75 @@ BOOL WINAPI SQLRemoveDSNFromIni(LPCSTR lpszDSN)
>       return FALSE;
>   }
>   
> -BOOL WINAPI SQLRemoveTranslatorW(LPCWSTR lpszTranslator, LPDWORD lpdwUsageCount)
> +BOOL WINAPI SQLRemoveTranslatorW(const WCHAR *translator, DWORD *usage_count)
>   {
> +    HKEY hkey;
> +    DWORD usagecount = 1;
> -
>
> clear_errors();
> -    FIXME("%s %p\n", debugstr_w(lpszTranslator), lpdwUsageCount);
> -    SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> -    return FALSE;
> +    TRACE("%s %p\n", debugstr_w(translator), usage_count);
> -
>
> +    if (RegOpenKeyW(HKEY_LOCAL_MACHINE, odbcini, &hkey) == ERROR_SUCCESS)
> +    {
> +        HKEY hkeydriver;
> -
>
> +        if (RegOpenKeyW(hkey, translator, &hkeydriver) == ERROR_SUCCESS)
> +        {
> +            DWORD size, type;
> +            DWORD count;
> -
>
> +            size = sizeof(usagecount);
> +            RegGetValueA(hkeydriver, NULL, "UsageCount", RRF_RT_DWORD, &type, &usagecount, &size);
> +            TRACE("Usage count %d\n", count);

It's a matter of taste, but is there a reason why you're mixing 16-bit functions (e.g. RegOpenKey) and functions defined for Vista and up (e.g. RegGetValue)?
You should use RegOpenKeyEx and RegQueryValueEx.

I realise this sort of mismatching occurs throughout odbccp32.c, but to my mind, it is wrong, and should be fixed file-wide in a separate patch.

I don't know how strongly Huw cares about this though.

Hugh

[v2 PATCH 2/3] odbccp32: Implement SQLRemoveTranslator/W

[Huw Davies]

On Tue, Apr 04, 2017 at 12:04:04PM +0000, Hugh McMaster wrote:
> It's a matter of taste, but is there a reason why you're mixing
> 16-bit functions (e.g. RegOpenKey) and functions defined for Vista
> and up (e.g. RegGetValue)?  You should use RegOpenKeyEx and
> RegQueryValueEx.
>
> I realise this sort of mismatching occurs
> throughout odbccp32.c, but to my mind, it is wrong, and should be
> fixed file-wide in a separate patch.>
>
> I don't know how strongly Huw cares about this though.

As far as I'm concerned, they're all part of the Win32 registry API
and can be used together as appropriate (with the exception of the
tests where we need to avoid APIs added after Win XP).

Huw.