Re: kernel32: fix PROFILE_Load to handle mac line endings

[Michael Karcher]

Am Sonntag, den 14.09.2008, 14:48 +0200 schrieb Erik Inge Bolsø:
> Fixes bug 15281. With testcase.
> @@ -407,6 +407,7 @@ static PROFILESECTION *PROFILE_Load(HANDLE hFile, ENCODING * pEncoding)
>      {
>          szLineStart = next_line;
>          next_line = memchrW(szLineStart, '\n', szEnd - szLineStart);
> -
>    if (!next_line) next_line = memchrW(szLineStart, '\r', szEnd - szLineStart);
>    if (!next_line) next_line = szEnd;
>    else next_line++;
>    szLineEnd = next_line;

This fails on files with mixed line endings. Alas, there is no mempbrk
or memcspn to help you scanning for both endings at the same time.

> @@ -415,7 +416,7 @@ static PROFILESECTION *PROFILE_Load(HANDLE hFile, ENCODING * pEncoding)
>      /* get rid of white space */
>      while (szLineStart < szLineEnd && PROFILE_isspaceW(*szLineStart)) szLineStart++;
> -
>    while ((szLineEnd > szLineStart) && ((szLineEnd[-1] == '\n') || PROFILE_isspaceW(szLineEnd[-1]))) szLineEnd--;
> -
>    while ((szLineEnd > szLineStart) && ((szLineEnd[-1] == '\n') || (szLineEnd[-1] == '\r') || PROFILE_isspaceW(szLineEnd[-1]))) szLineEnd--;
>
>    if (szLineStart >= szLineEnd) continue;

This hunk is unneded. PROFILE_isspaceW already checks for \n and \r. The
already present check for '\n' could be removed, but that should be a
separate patch. I will send it (together with a cleanup in
PROFILE_isspaceW) after testing.
> @@ -694,6 +694,273 @@ static void test_GetPrivateProfileString(void)
>      DeleteFileA(filename);
>  }
>
> +/* as above, but cr-only terminated lines */
> +static void test_cr(void)
> +{

[...]
Wouldn't it be sensible to factor out the common testing code, and just
pass the string that gets written to the file as parameter? (I didn't
check line-by-line that it matches, but the comment seems to implicate
it.) If you refactor, you could also add a test that uses mixed endings
(some \r, some \n, some \r\n).

Regards,
  Michael Karcher

kernel32: fix PROFILE_Load to handle mac line endings

[Erik Inge Bolsø]

On Sun, 14 Sep 2008, Michael Karcher wrote:
> This hunk is unneded. PROFILE_isspaceW already checks for \n and \r. The
> already present check for '\n' could be removed, but that should be a
> separate patch. I will send it (together with a cleanup in
> PROFILE_isspaceW) after testing.

Thanks. Indeed, tests still pass. Will remove the hunk and resend.
> [...]
> Wouldn't it be sensible to factor out the common testing code, and just
> pass the string that gets written to the file as parameter? (I didn't
> check line-by-line that it matches, but the comment seems to implicate
> it.) If you refactor, you could also add a test that uses mixed endings
> (some \r, some \n, some \r\n).

I'm just trying for a minimal fix and simple test for \r-terminated 
ini files here. If you want to clean it up further, feel free.

--
-erik	http://useofwords.blogspot.com/