On Wed, Oct 24, 2018 at 10:50 AM Huw Davies huw@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.