Re: [PATCH v2 3/5] comsvcs: Implement ISharedPropertyGroupManager_CreatePropertyGroup().

[Nikolay Sivov]

On 8/19/20 10:48 AM, Jactry Zeng wrote:
> +struct property_group
> +{
> - ISharedPropertyGroup ISharedPropertyGroup_iface;
> - struct group_manager *parent;
> - LONG isolation, release;
> - struct list entry;
> - LONG refcount;
> - BSTR name;
>
> +};

I think common formatting pattern wine uses is placing refcount field
after interfaces fields.
> +static ULONG WINAPI property_group_Release(ISharedPropertyGroup *iface)
> +{
> - struct property_group *group = impl_from_ISharedPropertyGroup(iface);
> - ULONG refcount = InterlockedDecrement(&group->refcount);
> -
> - TRACE("%p decreasing refcount to %u.\n", iface, refcount);
> -
> - if (!refcount)
> - {
> -
>    struct group_manager *manager = group->parent;
> -
> -
>    SysFreeString(group->name);
> -
> -
>    EnterCriticalSection(&manager->cs);
> -
>    list_remove(&group->entry);
> -
>    LeaveCriticalSection(&manager->cs);
> -
> -
>    ISharedPropertyGroupManager_Release(&manager->ISharedPropertyGroupManager_iface);
> - }
> -
> - return refcount;
>
> +}

This probably need to free group itself too.
> @@ -74,10 +248,21 @@ static ULONG WINAPI group_manager_Release(ISharedPropertyGroupManager *iface)
>  if (!refcount)
>  {
> -
>    if (!list_empty(&manager->property_groups))
> -
>    {
> -
>        struct property_group *group, *group2;
> -
> -
>        LIST_FOR_EACH_ENTRY_SAFE(group, group2, &manager->property_groups, struct property_group, entry)
> -
>        {
> -
>            list_remove(&group->entry);
> -
>            SysFreeString(group->name);
> -
>            heap_free(group);
> -
>        }
> -
>    }

There is no need for list_empty() test. Loop body would mostly duplicate
group's own Release() I imagine.
> +static struct property_group *find_propery_group(struct group_manager *manager, BSTR name)
> +{
> - struct property_group *group, *group2;
> -
> - LIST_FOR_EACH_ENTRY_SAFE(group, group2, &group_manager->property_groups, struct property_group, entry)
> - {
> -
>    if (!lstrcmpW(group->name, name))
> -
>        return group;
> - }
> - return NULL;
>
> +}

You don't need _SAFE() here.
> - {
> -
>    *exists = TRUE;
> -
>    *isolation = property_group->isolation;
> -
>    *release = property_group->release;
> -
>    hr = ISharedPropertyGroup_QueryInterface(&property_group->ISharedPropertyGroup_iface,
> -
>            &IID_ISharedPropertyGroup, (void **)group);
> - }

I think it's more straightforward to have assignment + addref instead,
as above. This will get rid of 'hr' too.