[PATCH 2/5] reg.exe: Add path_get_key() to remove boilerplate

[Hugh McMaster]

Hi Jonathan,

On Monday, 1 September 2014, 15:05:31 +0200, Jonathan Vollebregt wrote:

+ CompareStringW(CP_ACP,NORM_IGNORECASE,
+                path,strlenW(short_HKEY_name[i]),
+                short_HKEY_name[i],strlenW(short_HKEY_name[i]))==2 ||
+ CompareStringW(CP_ACP,NORM_IGNORECASE,
+                path,strlenW(long_HKEY_name[i]),
+                long_HKEY_name[i],strlenW(long_HKEY_name[i]))==2)

1. CompareString does not accept code page identifiers. It accepts a locale.[1] You could use LOCAL_USER_DEFAULT or GetThreadLocale() instead.

2. Please use CSTR_EQUAL instead of 2.

3. You should also avoid calling strlenW four times.

int len_short_HKEY_name = strlenW(short_HKEY_name[i]); or something similar.

Alternatively, since wine/unicode.h is already being included, consider using strncmpiW.

4. In patches 2 to 5, please mimic the style of the surrounding/original code. That being said, if there are variations in style, just use the most common style.

+    if(path_get_rootkey_name(path))
+    {
+        return HKEYs[(path_get_rootkey_name(path) - &long_HKEY_name[0][0]) / 20];
+    }
+    else
+    {
+        return NULL;
+    }

Single line code in if/else blocks can be compacted to use less space. E.g.:

if (path_get_rootkey_name(path))
    return HKEYs[(path_get_rootkey_name(path) - &long_HKEY_name[0][0]) / 20];
else
    return NULL;
> +static const WCHAR * path_get_rootkey_name(LPWSTR path)

Asterisks should be placed next to the object name or type, e.g. static const WCHAR *path_get_rootkey_name(LPWSTR path).
> - if(!key)
> - if (RegQueryValueW(key,value_name,NULL,NULL)==ERROR_SUCCESS)
> - RegSetValueExW(key,value_name,0,reg_type,reg_data,reg_count);

Also, code is easier to read if there is space after keywords, arguments and around == (equivalent to). You have been slightly inconsistent throughout.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/dd317759%28v=vs.85%2...

--
Hugh McMaster

[Jonathan Vollebregt]

> - if(path_get_rootkey_name(path))
> - {
> -
>    return HKEYs[(path_get_rootkey_name(path) - &long_HKEY_name[0][0]) / 20];
> - }
> - else
> - {
> -
>    return NULL;
> - }
>
> Single line code in if/else blocks can be compacted to use less space. E.g.:
>
> if (path_get_rootkey_name(path))
>      return HKEYs[(path_get_rootkey_name(path) - &long_HKEY_name[0][0]) / 20];
> else
>      return NULL;

Isn't it a best practice to always use braces?
> +static const WCHAR type_names[][MAX_TYPE_LENGTH] = {
> - {'R','E','G','_','N','O','N','E',0},
> - {'R','E','G','_','S','Z',0},
> - {'R','E','G','_','E','X','P','A','N','D','_','S','Z',0},
> - {'R','E','G','_','B','I','N','A','R','Y',0},
> - {'R','E','G','_','D','W','O','R','D',0},
> - {'R','E','G','_','D','W','O','R','D','_','B','I','G','_','E','N','D','I','A','N',0},
> - {0},
> - {'R','E','G','_','M','U','L','T','I','_','S','Z',0},
>
> +};
> -
> -
> -
>
> Also, what is the purpose of {0}?

REG_LINK isn't supported by anything in wine as far as I can tell, so I 
thought I'd just leave it empty. Would it be better to fill it in anyway?

[Hugh McMaster]

On Wed, 3 Sep 2014 13:15:43 +0200, Jonathan Vollebregt wrote:
> > Single line code in if/else blocks can be compacted to use less space. E.g.:
> >
> > if (path_get_rootkey_name(path))
> >      return HKEYs[(path_get_rootkey_name(path) - &long_HKEY_name[0][0]) / 20];
> > else
> >      return NULL;
>
> Isn't it a best practice to always use braces?

You could certainly argue that it is a best practice, and, in many ways, it is. But consider the if... else if... blocks from http://source.winehq.org/source/programs/reg/reg.c#0411 and following. If braces were included for each single line, each block would be far, far longer and (in my opinion) harder to read.
> > +static const WCHAR type_names[][MAX_TYPE_LENGTH] = {
> > - {'R','E','G','_','N','O','N','E',0},
> > - {'R','E','G','_','S','Z',0},
> > - {'R','E','G','_','E','X','P','A','N','D','_','S','Z',0},
> > - {'R','E','G','_','B','I','N','A','R','Y',0},
> > - {'R','E','G','_','D','W','O','R','D',0},
> > - {'R','E','G','_','D','W','O','R','D','_','B','I','G','_','E','N','D','I','A','N',0},
> > - {0},
> > - {'R','E','G','_','M','U','L','T','I','_','S','Z',0},
> >
> > +};
> > -
> > -
> > -
> >
> > Also, what is the purpose of {0}?
>
> REG_LINK isn't supported by anything in wine as far as I can tell, so I 
> thought I'd just leave it empty. Would it be better to fill it in anyway?

http://source.winehq.org/ident?_i=REG_LINK&_remember=1 shows that REG_LINK appears in the source in some form. Whether it is actively used in registry operations, I'm not sure.

Given that it appears, however, I'd include it anyway.