[PATCH v2 6/7] shell32/autocomplete: Use the optional IACList interface and IACList::Expand, if available

[Gabriel Ivăncescu]

On Mon, Oct 15, 2018 at 2:55 PM Huw Davies <huw at codeweavers.com> wrote:
>
> On Mon, Oct 15, 2018 at 02:24:01PM +0300, Gabriel Ivăncescu wrote:
> > On Mon, Oct 15, 2018 at 1:15 PM Huw Davies <huw at codeweavers.com> wrote:
> > >
> > > This is going to need some work; trying to follow this code is just too hard.
> > >
> > > Huw.
> >
> > The expand method gets called with the path ending in a slash, but
> > only if it's needed (the tests show this). For example, if the text is
> > a\b\c\d and the previous text was a\b\c\e, there's no expansion as
> > both would expand to a\b\c\ and Windows doesn't do it redundantly
> > (this is needed for things like paste or replacing selections to work
> > correctly, we can't just keep track of last character input, again,
> > the tests show this with selection deletions and caret movements and
> > others).
> >
> > Now what the code does is 3 "stages". First it skips all the shared
> > prefix (case insensitively). So basically the beginning of both
> > txtbackup and the new txt are skipped if identical (and stops where
> > they mismatch). At this point we know all characters before i (the
> > mismatch pos) are identical so no need to check them.
> >
> > Then, starting from the mismatch position, it looks for a delimiter in
> > the new text. If it finds one, obviously, it will look for the last
> > delim and expand the whole text up to that last delim (since they
> > differ).
> >
> > If it doesn't find one, it checks if txtbackup (the old text) has a
> > delim after they mismatch, because if they don't, it means they only
> > differ by non-delim characters, so no expansion is needed. (e.g.
> > a\b\c\foo and a\b\c\bar don't need any expansion)
> >
> > But if there is a delim in txtbackup (e.g. a\b\c\foo and
> > a\b\c\bar\baz), we need to find the last delim in the shared prefix,
> > so it scans backwards for the first delim (in this case, to expand
> > a\b\c\ since otherwise a\b\c\bar\ was expanded before).
> >
> > I know I could comment it better, but I don't want to be too verbose
> > so some advice would be appreciated.
>
> Well we've dicussed gotos already.  Admittedly this one isn't
> inside a nested switch, but still.
>
> You might also find strpbrkW() helpful.
>
> Huw.

Ah fair point, I guess I can use strpbrkW for the delims' search,
forgot about it.

But what's the problem with this goto? It's the same as failure gotos
which are used a lot in Wine's codebase. I used it because to me it
seems much cleaner of the purpose and the label's name (i.e. it
clearly expands given the last_delim, and also jumps to the end of the
function, no control flow issues).

The function itself is small and fits in one page so having an extra
helper function for this seems quite a bit overkill and less elegant
in encapsulation to me... (if C supported local/inner functions it
wouldn't be less elegant but of course IMO)

I guess I could also duplicate the expand code but that's even worse
for maintenance.