On Mon, Oct 15, 2018 at 2:55 PM Huw Davies huw@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@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.