On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura z.figura12@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