Re: dpnet: Implement IDirectPlay8Address GetComponentByIndex (try 4)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

You have style inconsistencies in the patch. I won't point them out individually and just focus on suggestions on the code itself.

Overall: Is there a way to remove components? I don't see a method that supports that, and AddComponent with a NULL pointer returns an error, so I guess components can't be removed.

Am 2015-01-06 um 08:59 schrieb Alistair Leslie-Hughes:> +static void add_component(IDirectPlay8AddressImpl *This, struct component *item)
> +{
> - if(!This->comp_count)
> -
>    This->components = HeapAlloc( GetProcessHeap(), 0, sizeof(struct component *) * This->comp_count+1 );
> - else
> -
>    This->components = HeapReAlloc( GetProcessHeap(), 0, This->components, sizeof(struct component *) * This->comp_count+1 );

How many components are usually added? If it is just a small number (say, < 10) this is OK. Otherwise it would be more efficient to grow the array by a factor (e.g. duplicate the array size each time you grow it), so you make growing an O(log(n)) instead of O(n).
> -
>    int i;
> -
>    struct component *entry;
>
> -
>    LIST_FOR_EACH_ENTRY_SAFE(entry, entry2, &This->components, struct component, entry)
> -
>    for(i=0; i < This->comp_count; i++)

Mismatching data type. This->comp_count is a DWORD, i is an int. Please use DWORD consistently. (Using DWORD for the component is mandated by the parameters to GetComponentByIndex and return value of GetNumComponents)
> - switch (entry->type)
> - {
> -
>    case DPNA_DATATYPE_DWORD:
> -
>        memcpy(pvBuffer, &entry->data.value, sizeof(DWORD));
> -
>        break;
> -
>    case DPNA_DATATYPE_GUID:
> -
>        memcpy(pvBuffer, &entry->data.guid, sizeof(GUID));
> -
>        break;
> -
>    case DPNA_DATATYPE_STRING:
> -
>        memcpy(pvBuffer, &entry->data.string, entry->size);
> -
>        break;
> -
>    case DPNA_DATATYPE_STRING_ANSI:
> -
>        memcpy(pvBuffer, &entry->data.ansi, entry->size);
> -
>        break;
> -
>    case DPNA_DATATYPE_BINARY:
> -
>        memcpy(pvBuffer, &entry->data.binary, entry->size);
> -
>        break;
> - }

You can use assignments for DPNA_DATATYPE_DWORD and DPNA_DATATYPE_GUID. The same applies to the existing code in GetComponentByName. AddComponent already uses assignments.
> --- a/dlls/dpnet/dpnet_private.h
> +++ b/dlls/dpnet/dpnet_private.h
> @@ -94,7 +94,8 @@ struct IDirectPlay8AddressImpl
>      GUID SP_guid;
>      BOOL init;
> - struct list components;
> - struct component **components;
> - DWORD comp_count;

Do you need the extra indirection? If the number of components is small you can work without it I think.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJUq/bOAAoJEN0/YqbEcdMwOnMP/RnnAWVfyTuK2ABnRqDLW9/1
DydTHhjeOpm1FDKJTy7ebzqU76HDL7YWJOqwSNYPhtr8e3omXt5nApptwVjOBst0
lpRxDju4sPGo9GYePy5w39TI/w78gPLEGLVHpIiV5GlMz+YRpgxKI05Q90jkGtfo
whPpQ9ns1xiXz+d07iJnPn8GnEjWeTkIN2NnY6Nj+ARQ8rmUNSn3YR4ZGtZfuzrt
nKUurOt04FUAqQzDNYNbdtpybYQq7SSVQr8vWpJAJOpedq7oNyCEdjC8Zl5Yitl5
bg6a4oTvl7PgGmYOSvNVzsh6L0dYv0HpjLZsfLUo97ULXRsCFgfX8yNogrCSq8o0
2KoZSWEhehn1RV+WCywMbxHxly5ZvkyEr9g9/h+8+7cpPrdg8TeoaMUjtjUxrqra
fLDfLtGnMGs4DGiFmleRR3lBFmr8UMVkP4/YLEJ1HPKNdAAwXfQso6/ODmdvC78W
5sRfY/pSlLEn93szGfx+hunpUX30c2XkGmuBElLJQsQgVeoijmsNhUNOLUdhTx9k
wPhslaQ+HCtR76KVghvQzN3iBHPuO+O0dfH4PWz5/7Grq5/pIhfbWcMCVJJM3d9u
CNjKAOkw+NzPPsdLyJZsmMWWvv+BJ039l15xjMTmWxG2uH/QgaiBtvDyD2/u3KxJ
MgsfOYAWEYondC/Jllaz
=uIjr
-----END PGP SIGNATURE-----