Re: [PATCH 1/6] shell32/autocomplete: Introduce helpers for proper enumeration

[Huw Davies]

On Tue, Oct 23, 2018 at 02:26:02PM +0300, Gabriel Ivăncescu wrote:
> These will be used to implement proper string enumeration, because Windows
> doesn't reset and re-enumerate it everytime autocompletion happens, and it
> also sorts them like Windows does.
> Signed-off-by: Gabriel Ivăncescu
>
> For now, the first two patches in the series do nothing because the helpers
> are unused; they will be used afterwards, and were split to avoid the
> patches becoming too large.

Please don't do this.  Every patch needs to stand by itself; adding unused
code doesn't count.
> Merge Sort has been used to sort the strings because it is stable, and
> because it minimizes the amount of comparisons, which is essential for
> case-insensitive string comparisons.

I don't see why do you need a stable sort here.  Using libc's qsort
should be fine at first.
> +static void enumerate_strings(IAutoCompleteImpl *ac)
> +{
> - /*
> -
>   Enumerate all of the strings and sort them in the internal list. Rough summary:
> -
> -
>     - Enumerate the strings and place their addresses in a chain of blocks (stack-like)
> -
>     - Sort pairs from the chain of blocks into a contiguous array of pointers to them
> -
>     - Merge Sort the contiguous array/list (excluding pairs, since it's already done)
> -
> -
>   We don't free the enumerated strings (except on error) to avoid needless
> -
>   copies, until the next reset (or the object itself is destroyed)
> - */
> - struct
> - {
> -
>    void *prev;
> -
>    LPOLESTR str[511 * 2];  /* this must be kept *even* */
> - } *prevblock, *curblock;
> -
> - UINT i, cur, numstrs = 0;
> - LPOLESTR *strs;
> -
> - prevblock = NULL;
> - for (;;)
> - {
> -
>    LONG rem;
> -
>    if ((curblock = heap_alloc(sizeof(*curblock))) == NULL)
> -
>    {
> -
>        if (!prevblock)
> -
>            return;
> -
>        curblock = prevblock;
> -
>        cur = ARRAY_SIZE(curblock->str);
> -
>        goto fail_and_free_blocks;
> -
>    }
> -
>    curblock->prev = prevblock;
> -
>    rem = ARRAY_SIZE(curblock->str);
> -
> -
>    while (rem > 0)
> -
>    {
> -
>        ULONG n = 0;
> -
>        cur = ARRAY_SIZE(curblock->str) - rem;
> -
>        IEnumString_Next(ac->enumstr, rem, &curblock->str[cur], &n);
> -
>        if (n == 0)
> -
>            goto break_from_enumeration;
> -
>        rem -= n;
> -
>    }
> -
>    prevblock = curblock;
> -
>    numstrs += ARRAY_SIZE(curblock->str);
> - }
> -
>
> +break_from_enumeration:
> - /* Allocate even if there were zero strings enumerated, to mark it non-NULL */
> - numstrs += cur;
> - if (!(strs = heap_alloc(numstrs * sizeof(*strs))))
> - {
> -
>  fail_and_free_blocks:
> -
>    do
> -
>    {
> -
>        LPOLESTR *str = curblock->str;
> -
>        while (cur--)
> -
>            CoTaskMemFree(str[cur]);
> -
>        prevblock = curblock->prev;
> -
>        heap_free(curblock);
> -
>        curblock = prevblock;
> -
>        cur = ARRAY_SIZE(curblock->str);
> -
>    } while (curblock);
> -
>    return;
> - }

And even though I've mentioned it twice already you're still using
gotos.  Please get them out of your system; there's no way anyone is
going sign off on this.  A 'goto fail' is ok, but the fail label needs
to be at the outer-level and near the end of the function.  If you
think you need a goto for anything else then it's a sign you need to
restructure your code.

Huw.