[PATCH v2] ws2_32: Drop dependencies on system getprotoby(name|number) functions

[Alex Henrie]

 On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>
> > +    /* keep this list sorted by number for binary search */
>
> This seems to me like it should be a separate patch.

Why? I have to change the implementation of getprotobynumber anyway,
and I don't want to cause a performance regression, so it seems that I
might as well put in the binary search at the same time.

> If binary search is worth implementing, can we just use bsearch() to do it?

Probably yeah. I'll give that a try.

> > +    const char *names[3];
>
> I guess this was copied from the implementation, but surely this can
> just be names[2]?

The NULL name at the end is used as a sentinel in the tests, the same
as in the implementation:

            for (j = 0; protocols[i].names[0][j]; j++)

> > +    { }
>
> If you're using ARRAY_SIZE, does this need to be null-terminated?

Yes, it's used as a sentinel in the tests:

       if (i != ref->prot)

> > +        if (protocols[i].names[0])
>
> Isn't this condition always true?

Yes it is. I meant to remove this condition in v2. I will fix that in
v3. Great catch!

> > +            ok(!ent,
>
> This seems like an odd place for a line break.

It's meant to mirror the format of the other ok statements in
test_getprotobyname and test_getprotobynumber. But it's just a style
thing; I'll delete the line break here if more people say that they
don't like it.

> I guess this test structure isn't too confusing on examination, but it
> might be clearer just to have a semi-reimplementation of
> getprotobynumber() in the tests, to which you compare the actual data.

The test is already a semi-reimplementation of getprotobynumber: It
has a copy of the protocol table that is identical to the
implementation's. How is what you're suggesting different?

-Alex