Re: [PATCH v3 2/5] strvec: add a strvec_pushvec()

From: Jeff King <peff@peff.net>
Date: Aug 28, 2021, 04:12 UTC
Message-ID: <YSm3ofxlRB1ViBf5@coredump.intra.peff.net>
In-Reply-To: <xmqq4kba9x1h.fsf@gitster.g>

On Fri, Aug 27, 2021 at 06:29:30PM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +void strvec_pushvec(struct strvec *array, const struct strvec *items)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < items->nr; i++)
> >> +		strvec_push(array, items->v[i]);
> >> +}
> >
> > This implementation is not wrong per-se, but is somewhat
> > disappointing.  When items->nr is large, especially relative to the
> > original array->alloc, it would incur unnecessary reallocations that
> > we can easily avoid by pre-sizing the array before pushing the
> > elements of items from it.
> >
> > In the original code that became the first user of this helper, it
> > may not have made much difference, but now it is becoming a more
> > generally reusable API function, we should care.
> 
> And if we do not care, you can rewrite the code that became the
> first user of this helper to instead call strvec_pushv() on the
> items->v array that is guaranteed to be NULL terminated, without
> inventing this new helper.

I came here to say that. ;)

I do not mind using pushv() directly, or a pushvec() that is a convenience wrapper for pushv(). Even better if that wrapper is smart enough to pre-allocate based on items->nr, as you mentioned, but that can also come later.

One thing that did surprise me: the use of "int" here for iterating, rather than size_t. But it seems that strvec is already storing ints, which is an accident!

I think we'd want the patch below. It can be applied independently (though if we do take the index-iterating version of Ævar's patch, I think it should switch to size_t).

-- >8 --
Subject: [PATCH] strvec: use size_t to store nr and alloc

We converted argv_array (which later became strvec) to use size_t in 819f0e76b1 (argv-array: use size_t for count and alloc, 2020-07-28) in order to avoid the possibility of integer overflow. But later, commit d70a9eb611 (strvec: rename struct fields, 2020-07-28) accidentally converted these back to ints!

Those two commits were part of the same patch series. I'm pretty sure what happened is that they were originally written in the opposite order and then cleaned up and re-ordered during an interactive rebase. And when resolving the inevitable conflict, I mistakenly took the "rename" patch completely, accidentally dropping the type change.

We can correct it now; better late than never.

Signed-off-by: Jeff King <peff@peff.net>
---
 strvec.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/strvec.h b/strvec.h
index fdcad75b45..6b3cbd6758 100644
--- a/strvec.h
+++ b/strvec.h
@@ -29,8 +29,8 @@ extern const char *empty_strvec[];
  */
 struct strvec {
 	const char **v;
-	int nr;
-	int alloc;
+	size_t nr;
+	size_t alloc;
 };
 
 #define STRVEC_INIT { empty_strvec, 0, 0 }

-- 
2.33.0.399.g06f2549587