On Tue, Apr 18, 2023 at 03:18:49PM -0400, Taylor Blau wrote:
> @@ -159,21 +161,34 @@ int cmd__hashmap(int argc, const char **argv)
>
> /* process commands from stdin */
> while (strbuf_getline(&line, stdin) != EOF) {
> - char *cmd, *p1 = NULL, *p2 = NULL;
> + char *cmd, *p1, *p2;
> unsigned int hash = 0;
> struct test_entry *entry;
>
> + /*
> + * Because we memdup() the arguments out of the
> + * string_list before inserting them into the hashmap,
> + * it's OK to set its length back to zero to avoid
> + * re-allocating the items array once per line.
> + *
> + * By doing so, we'll instead overwrite the existing
> + * entries and avoid re-allocating.
> + */
> + string_list_setlen(&parts, 0);
> /* break line into command and up to two parameters */
> - cmd = strtok(line.buf, DELIM);
> + string_list_split_in_place_multi(&parts, line.buf, DELIM, 2);
> +
I'd argue we can drop this comment now. Having string_list_setlen()
makes it a blessed pattern, and I don't think there's anything special
about this caller that makes it more or less so. Obviously yes, the
string list items won't be valid as we enter a new loop iteration. But
that is always true of split_in_place(), not to mention strtok(),
because we are overwriting the buffer in each loop.
Ditto for the later commits which have similar (if shorter) comments.
-Peff
