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

Mon Mar 2 04:28:03 CST 2015

-----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.