[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 <gabrielopcode at gmail.com>
> ---
> 
> 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.