[PATCH v8 2/3] shell32: Implement FolderItems_Item.

[Sebastian Lackner]

On 22.08.2016 18:09, Alex Henrie wrote:
>>> +
>>> +            if (!PathCombineW(path, V_BSTR(dir), file_info.cFileName))
>>> +            {
>>> +                ret = E_OUTOFMEMORY;
>>> +                goto fail;
>>> +            }
>>
>> I think it makes more sense to store the relative paths, there is no
>> need to prepend the directory name in advance. FolderItemsImpl_Item
>> also expects a relative path for example.
> 
> Are you sure? In the common case, FolderItemsImpl_Item receives a
> VT_I4 index and can simply pass the saved path from the array to
> FolderItem_Constructor. If we save filenames instead, we'll have to
> call PathCombineW, SysAllocString, and SysFreeString in both the VT_I4
> and the VT_BSTR cases.

I definitely think this is better. It is wasting a lot of memory when
the full path is computed in advance. Also, it would be more consistent
with the existing calls to FolderItem_Constructor(). If you are concerned
about having the code twice, you could either implement it in the
constructor itself (and simplify the existing code) or by using a common
implementation for both VT_I4 and VT_BSTR. Something like this should work
(pseudo-code):

--- snip ---
    switch (V_VT(&index))
    {
        case VT_I2:
            i = V_I2(&index);
            break;

        case VT_I4:
            i = V_I4(&index);
            break;

        case VT_BSTR:
            [...]
            break;

        case VT_ERROR:
            return FolderItem_Constructor(&This->dir, ppid);

        default:
            return E_NOTIMPL;
    }

    if (i < 0 || i >= This->item_count)
        return S_FALSE;

    [... call constructor ...]
--- snip ---

Please note that after this change it might no longer be necessary to
use SysAllocString for the internal variables.

> 
>>> +
>>> +            paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (This->item_count + 1) * sizeof(BSTR));
>>
>> It seems more reasonable to increase the buffer size in bigger chunks.
>> Its better to have a few entries overhead than to do frequent reallocs.
> 
> How about allocating 128 string pointers (1 kibibyte) at a time? The
> code would be:
> 
> if (This->item_count % 128)
> {
>     paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths,
>                         (1 + This->item_count / 128) * 128 * sizeof(BSTR));
>     if (!paths)
>         goto fail;
>     This->item_paths = paths;
> }

It is probably fine, but actually it might be easier to just add a "item_size"
variable corresponding to the current amount of allocated memory. For example:

--- snip ---
    item_size = 128;
    This->item_paths = HeapAlloc(GetProcessHeap(), 0, item_size * sizeof(...));
    [...]
            if (This->item_count >= item_size)
            {
                paths = HeapReAlloc(GetProcessHeap(), 0, This->item_paths, (item_size + 128) * sizeof(...));
                if (!paths)
                {
                    ret = E_OUTOFMEMORY;
                    goto fail;
                }
                This->item_paths = paths;
                item_size += 128;
            }
--- snip ---