Re: [PATCH 1/3] ntdll/tests: Tests for RtlIpv6StringToAddress (try 4)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Am 2015-03-02 um 00:08 schrieb Mark Jansen:
> +    for (n = 0; name_a[n]; ++n)
> +        name[n] = name_a[n];
> +    name[n] = 0;
> As discussed on IRC please use RtlMultiByteToUnicodeN here. Other tests use it for similar purposes, so I think it is OK to load and use here as well. Note that the other tests assume the function is available, so you don't need to add a skip() check for it.

There are a few things where I don't know if Alexandre cares about them: The long lines (it may make sense to add some line breaks e.g. in long table entries), adding things like "ex_fails" in patch 1 rather than patch 2 where they are actually used (I think it's better to keep it your way, moving this to the other patch will be lots of work and won't improve readability), and the W test could in theory be moved to a separate patch. Personally I am fine with keeping those things the way they currently are.

- From Patch 2:
> +        init_ip6(&expected_ip, ipv6_tests[i].ip);
> +
> +        /* we do not test ip if the ex has a special case failure here. */
> +        if (res == expect_ret)
> +        {
> +            ok(!memcmp(ip.s6_addr, expected_ip.s6_addr, sizeof(ip._S6_un)),
> +                "[%s] ip = %x:%x:%x:%x:%x:%x:%x:%x, expected %x:%x:%x:%x:%x:%x:%x:%x\n",
> I'm not sure what the comment means. Afaics you're always checking the IP except in case of win_broken_6. ex_fail_6 doesn't matter here.

You can move the init_ip6 call into the if branch since you're not using it otherwise.

- From Patch 3:
> +        //ok(terminator == ipv4_tests[i].address + ipv4_tests[i].terminator_offset,
> +        //   "[%s] terminator = %p, expected %p\n",
> +        //   ipv4_tests[i].address, terminator, ipv4_tests[i].address + ipv4_tests[i].terminator_offset);
> Leftover code from the A test? Since Ex doesn't write the terminator you can just remove that I think.

Personally I am not too fond of the test changing its own input data (i.e., the !(ipv4_tests[i].flags & strict_diff_4) case), but since this is existing code I'd say keep it that way.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU9DszAAoJEN0/YqbEcdMwo6AP/jU5NK5YqSXGuUYi1eOoaSAu
23nR4KvoaS/lXnPsZ2QPm+kyeSsWynwj0UYZ4dJJczve81m+5YYDLe4zc5Io/Fz0
C4x8WIMiBGPbhYM+iyIVAItz1wnL/9sB2IrlocQ/N9gYU1m25FLMBLvORePfQQs5
UEOOeZbaws7Qd8n+XDR7JRY9e946B7OVG8TFfq4in8IlPUsmmy+m65jQXGgQeBcF
JZgOqJH+fSqW0yf3sHhok9wj7KrRpDVUgBlowBXooBz5uuEJwbQ3IwlrbL28PNIj
PylUAHdVjeoHmKRif8gykwOt0laOe+RenSxegJbYbz3nw/d9Jmicj5n6W0W92hvb
EPMR0a/HbNF2P6TKQIKSMwoaKhtxRXzfcgBz3Ff3LH63qtRYHxMAneU/yaueBMxR
ZmfgWT9FHofH5dk2b4rRuP8XhKB/UFyqhlmimIBeDATO/aasNYuRjYlmce1NnfDa
pQqW+ntq/H8TEz1jc2IQS0dm9iQZITSMkQnyJdcMi2GTQEh8y41wa0MuOTC+DavF
8HqmTmo7+/VeaZfAmWx1ndtrSktcPDr4wM70aYPcRP1+JyLsLDfkoV5VDXaelZc2
PwH1MBp6ZTv8t5ckaer4wCpvuDH972/jjB56FnbRF1SiFoYcMEs+ZuHHF5YpMn5A
Pd6IazMRvRk6dd+ewaSJ
=aaY5
-----END PGP SIGNATURE-----