[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.