Jeff King <peff@xxxxxxxx> writes:
> Our callback function does _one_ type of selection (based on a "type"
> parameter), but not another (based on the name). That feels
> inconsistent, but is also the reason we have this awkward struct.
I wrote a very similar review but did not send it out, as I couldn't
pinpoint where the awkwardness come from exactly.
> Part
> of my confusion is the name: this is not something to be generically
> used with for_each_userdiff_driver(), but rather a type unique to
> find_by_namelen() to be passed through the opaque void pointer.
>
> So "struct find_by_namelen_data" would have been a lot more
> enlightening.
Yes. The callback parameter being "void *" is the API, and it is
just this user that uses this particular structure.
And while "type" could also be made a part of this structure and
have the API always iterate over all entries regardless of "type",
i.e. the callback function could be the one responsible for finding
the entries in the table for one particular type, it is understandable
that potential callers can be helped by having the pre-filtering
based on the "type" thing on the API side.
>> + if (!strncmp(driver->name, cb_data->k, cb_data->len) &&
>> + !driver->name[cb_data->len]) {
>> + cb_data->driver = driver;
>> + return -1; /* found it! */
>> }
>
> This "return -1" took me a while to grok, and the comment didn't help
> all that much. The point is to stop traversing the list, but "-1" to me
> signals error. I think returning "1" might be a bit more idiomatic, but
> also a comment that says "tell the caller to stop iterating" would have
> been more clear.
And the fact that it becomes the return value of "for_each_()" iterator
does not quite help to use a negative value, implying there was some
error condition X-<.
> Perhaps it would make more sense as:
>
> USERDIFF_DRIVER_TYPE_BUILTIN = 1<<0,
> USERDIFF_DRIVER_TYPE_CUSTOM = 1<<0,
> USERDIFF_DRIVER_TYPE_ALL = USERDIFF_DRIVER_TYPE_BUILTIN | USERDIFF_DRIVER_TYPE_CUSTOM
>
> Or the one caller who wants "ALL" could even do the OR themselves.
Great minds think alike ;-)
> I do kind of wonder if there's much value in having a single function
> with a type field at all, though, given that there's no overlap in the
> implementation. Would separate "for_each_custom" and "for_each_builtin"
> functions make sense? And then the existing caller would just call them
> sequentially.
Or a single for_each_driver() that gives <name, length, type> tuple
to the callback function, iterating over all drivers regardless of
the type.
