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.