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

[Nikolay Sivov]

On 8/30/20 6:03 PM, Jactry Zeng wrote:
> static HRESULT WINAPI group_manager_CreatePropertyGroup(ISharedPropertyGroupManager *iface, BSTR name, LONG *isolation,
>          LONG *release, VARIANT_BOOL *exists, ISharedPropertyGroup **group)
>  {
> - FIXME("iface %p, name %s, isolation %p, release %p, exists %p, group %p: stub.\n",
> - struct group_manager *manager = impl_from_ISharedPropertyGroupManager(iface);
> - struct property_group *property_group;
> -
> - TRACE("iface %p, name %s, isolation %p, release %p, exists %p, group %p.\n",
>         iface, debugstr_w(name), isolation, release, exists, group);
> - return E_NOTIMPL;
> -
> - if (!name)
> -
>    return E_POINTER;
> -
> - if (*isolation > 1 || *release > 1)
> -
>    return E_INVALIDARG;
> -
> - if (*isolation || *release)
> -
>    FIXME("Unsopported mode: isolation %d, release %d.\n", *isolation, *release);

They have named constants for modes. Also typo in fixme message.
> -
> - EnterCriticalSection(&manager->cs);
> -
> - property_group = find_propery_group(manager, name);
> - if (!property_group)
> - {
> -
>    property_group = heap_alloc(sizeof(*property_group));
> -
>    if (!property_group)
> -
>    {
> -
>        LeaveCriticalSection(&manager->cs);
> -
>        return E_OUTOFMEMORY;
> -
>    }
> -
>    property_group->ISharedPropertyGroup_iface.lpVtbl = &property_group_vtbl;
> -
>    property_group->parent = manager;
> -
>    property_group->isolation = *isolation;
> -
>    property_group->release = *release;
> -
>    property_group->refcount = 1;
> -
>    property_group->name = SysAllocString(name);
> -
>    if (!property_group->name)
> -
>    {
> -
>        LeaveCriticalSection(&manager->cs);
> -
>        heap_free(property_group);
> -
>        return E_OUTOFMEMORY;
> -
>    }
> -
> -
>    list_add_tail(&manager->property_groups, &property_group->entry);
> -
> -
>    *exists = FALSE;
> -
>    ISharedPropertyGroupManager_AddRef(&property_group->parent->ISharedPropertyGroupManager_iface);
> - }
> - else
> - {
> -
>    *exists = TRUE;
> -
>    *isolation = property_group->isolation;
> -
>    *release = property_group->release;
> -
>    ISharedPropertyGroup_AddRef(&property_group->ISharedPropertyGroup_iface);
> - }
> - *group = &property_group->ISharedPropertyGroup_iface;
> -
> - LeaveCriticalSection(&manager->cs);
> -
> - return S_OK;

I think it would be more readable if lock/unlock was around a smaller
block, for example using a helper that creates a new group instance, to
avoid multiple error handling branches.