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