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

[Gabriel Ivăncescu]

On Wed, Oct 24, 2018 at 10:50 AM Huw Davies <huw at codeweavers.com> wrote:
>
> Please don't do this.  Every patch needs to stand by itself; adding unused
> code doesn't count.
>

So I should merge them into just one big patch then? (the first 3 in the series)

>
> I don't see why do you need a stable sort here.  Using libc's qsort
> should be fine at first.
>

Because the strings are compared case insensitively, so keeping the
order in which they are originally enumerated is important to
distinguish this, since some apps want to have them in a certain way
(and probably even provide them already sorted, for example, like
files on a case-insensitive filesystem, i.e. Total Commander, which
should be in the exact same way).

Furthermore, qsort would actually be slower still, since it doesn't
take advantage of the fact that pairs are already sorted when moving
them off the chain of blocks into the contiguous array. In some cases
like my use case, I think it's significantly slower (~13%) and would
be *very* slow on systems without a merge sort (because the string
comparisons are the bottleneck, by far, *especially* due to caching
reasons). These aren't random benchmarks with inflated time numbers,
I've literally profiled it for my use case, not with artificial code
and loops.

(of course, it's still somewhat unusable since populating the listbox
would be the next bottleneck to take care of)

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

But one of the goto *is* a failure goto though? The
"fail_and_free_blocks" is literally a failure and cleanup goto as its
name says. Yes, it does jump into a block, but that block *returns* at
the end and I've done it this way to avoid indenting everything else
after it, which is literally the same as jumping to the end? So
instead of:

if (error)
{
  fail:
    /* free stuff */
    return;
}

/* non-error path */

It would look like:

if (!error)
{
    /* non-error path */
    return;
}

fail:
/* free stuff */

Which would indent the entire non-error path for no reason and I think
it is worse off. Note that you can't "free stuff" after the non-error
path because the non-error path has to free them as it actually uses
them later, but not at the end (they blocks are already gone by that
point).


Lastly, the other goto is literally a "break twice" goto to break out
of nested scope, which is normal in many code bases and I thought it's
not an issue in the least. It also breaks *outside* any scopes and in
fact many files in the wine code-base do that as well (e.g.
user32/win.c with "goto other_process" is literally a goto to break
out of inner scopes and jumps *far* further than this goto, why is
that one ok?).

I fail to see why a normal break is different than this goto, and if C
supported nested breaks, it would be literally identical to a "break
label" or the like. But maybe you only had issue with the first goto?

The alternative would be to add BOOL checks on the outer scope but IMO
that's way more ugly and requires more code.