On Sat, Apr 22, 2023 at 07:16:57AM -0400, Jeff King wrote:
> 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.
Agreed, I think that part of the point of string_list_setlen() is that
this is a blessed pattern, so shouldn't need a comment.
Thanks,
Taylor
