Re: StrCatChainW implementation

[Nikolay Sivov]

On 3/6/2014 15:17, Huw Campbell wrote:
> Hello
>
> Please see my implementation of this function, which  has not been 
> seen in wine previously. Explorer.exe calls it, so it's kind of important.
>
> I ran a lot of tests on the edge cases: including: same ends, 0 ends, 
> end beyond the length...

Hi, Huw.

We need those tests in shlwapi/tests. Now regarding patch itself.
> +DWORD WINAPI StrCatChainW(PWSTR pszDst, DWORD cchDst, DWORD ichAt, PCWSTR pszSrc)

Please use lower case naming that's more readable, like 'dest', 
'dest_len', 'at', 'src'.
> +{
> - LPWSTR d = pszDst;
> - LPCWSTR s = pszSrc;

It doesn't look like you need this 'd' and 's', you could use passed 
pointers.
> - DWORD mark;
> -
> - TRACE("(%s,%i,%i,%s)\n", debugstr_w(pszDst), cchDst, ichAt, debugstr_w(pszSrc));

Is it possible that destination buffer is uninitialized? In this case 
you can't trace in like this.
> -
> - if (ichAt == -1)
> - mark = strlenW(pszDst);
> - else
> - mark = ichAt;
> - if (mark > cchDst || cchDst == 0)
> - return mark;
> - if (mark == cchDst)
> - mark--;
> - d = d + mark;
> - if (pszSrc)
> - {
> - while ((mark < cchDst-1) && *s)
> - {
> -
>  mark++;
> -
>  *d++ = *s++;
> - }
> - }
> - *d = 0;

What happens if 'd' is null? Could this loop be replaced with StrNCatW 
or something similar?
> - return mark;
>
> +}