Hi Junio,
On 2015-06-19 21:26, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
>> +static inline int substrcmp(const char *string, int len, const char *match)
>> +{
>> + int match_len = strlen(match);
>> + if (match_len != len)
>> + return -1;
>> + return memcmp(string, match, len);
>> +}
>
> What this does looks suspiciously like starts_with(), but its name
> "substrcmp()" does not give any hint that this is about the beginnig
> part of "string"; if anything, it gives a wrong hint that it may be
> any substring. prefixcmp() might be a better name but that was the
> old name for !starts_with() so we cannot use it here. It is a
> mouthful, but starts_with_counted() may be.
It is actually not `prefixcmp()` because it requires that `match` has precisely `len` bytes, while `prefixcmp()` or `starts_with()` would not care as long as the first `strlen(match)` bytes of `string` match `match`.
Maybe `fixed_length_strcmp()` would be an appropriate name, but it is pretty long and *still* does not convey exactly what this function is about.
Also please note that this is a `static inline` function, i.e. its definition does not bleed outside of this file.
So I hope that you are satisfied with simply adding
/** compares a counted string to a NUL-terminated one. */
as comment above the `substrcmp()` function?
> But the whole thing may be moot.
>
> If we take the "why not upcase the end-user string upfront"
> suggestion from the previous review, fsck_set_msg_types() would have
> an upcased copy of the end-user string that it can muck with; it can
> turn "badfoo=error,poorbar=..." into "BADFOO=error,POORBAR=..."
> that is stored in its own writable memory (possibly a strbuf), and
> at that point it can afford to NUL-terminate BADFOO=error after
> finding where one specification ends with strcspn() before calling
> fsck_set_msg_type(), which in turn calls parse_msg_type().
Hmm. I really do not like that kind of thinking, i.e. having to duplicate, then modify data to be able to call the API, only to have to modify the data back afterwards, and eventually having to unallocate the data in all code paths. That feels just very inelegant to me.
I agree that it cannot be helped, sometimes, when you *have* to do exactly such mucking with data on a copy, be it to avoid additional complexity or poor performance. But I simply do not see the need for that here.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in