Re: [PATCH v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 25 Nov 2018, Thomas Gummerer wrote:

> On 11/23, Paul-Sebastian Ungureanu wrote:
> > Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
> > insert data using a printf format string.
> > 
> > Original-idea-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> > ---
> >  strbuf.c | 36 ++++++++++++++++++++++++++++++++++++
> >  strbuf.h |  9 +++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/strbuf.c b/strbuf.c
> > index 82e90f1dfe..bfbbdadbf3 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
> >  	strbuf_splice(sb, pos, 0, data, len);
> >  }
> >  
> > +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)
> > +{
> > +	int len, len2;
> > +	char save;
> > +	va_list cp;
> > +
> > +	if (pos > sb->len)
> > +		die("`pos' is too far after the end of the buffer");
> 
> I was going to ask about translation of this and other messages in
> 'die()' calls, but I see other messages in strbuf.c are not marked for
> translation either.  It may make sense to mark them all for
> translation at some point in the future, but having them all
> untranslated for now makes sense.
> 
> In the long run it may even be better to return an error here rather
> than 'die()'ing, but again this is consistent with the rest of the
> API, so this wouldn't be a good time to take that on.

I guess I was too overzealous in my copying. These conditions really
indicate bugs in the caller... So I'd actually rather change that die() to
BUG().

But then, the original code in strbuf_vaddf() calls die() and would have
to be changed, too.

> > +	va_copy(cp, ap);
> > +	len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
> 
> Here we're just getting the length of what we're trying to format
> (excluding the final NUL).  As the second argument is 0, we do not
> modify the strbuf at this point...
> 
> > +	va_end(cp);
> > +	if (len < 0)
> > +		BUG("your vsnprintf is broken (returned %d)", len);
> > +	if (!len)
> > +		return; /* nothing to do */
> > +	if (unsigned_add_overflows(sb->len, len))
> > +		die("you want to use way too much memory");
> > +	strbuf_grow(sb, len);
> 
> ... and then we grow the strbuf by the length we previously, which
> excludes the NUL character, plus one extra character, so even if pos
> == len we are sure to have enough space in the strbuf ...
> 
> > +	memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
> > +	/* vsnprintf() will append a NUL, overwriting one of our characters */
> > +	save = sb->buf[pos + len];
> > +	len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
> 
> ... and we use vsnprintf to write the formatted string to the
> beginning of the buffer.

It is not actually the beginning of the buffer, but possibly the middle of
the buffer ;-)

> sb->alloc - sb->len can be larger than 'len', which is fine as vsnprintf
> doesn't write anything after the NUL character.  And as 'strbuf_grow'
> adds len + 1 bytes to the strbuf we'll always have enough space for
> adding the formatted string ...
> 
> > +	sb->buf[pos + len] = save;
> > +	if (len2 != len)
> > +		BUG("your vsnprintf is broken (returns inconsistent lengths)");
> > +	strbuf_setlen(sb, sb->len + len);
> 
> And finally we set the strbuf to the new length.  So all this is just
> a very roundabout way to say that this function does the right thing
> according to my reading (and tests).

:-)

It seems that Junio likes this way of reviewing, giving him confidence
that the review was thorough.

Thanks!
Dscho

> > +}
> > +
> > +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	va_start(ap, fmt);
> > +	strbuf_vinsertf(sb, pos, fmt, ap);
> > +	va_end(ap);
> > +}
> > +
> >  void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
> >  {
> >  	strbuf_splice(sb, pos, len, "", 0);
> > diff --git a/strbuf.h b/strbuf.h
> > index be02150df3..8f8fe01e68 100644
> > --- a/strbuf.h
> > +++ b/strbuf.h
> > @@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
> >   */
> >  void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
> >  
> > +/**
> > + * Insert data to the given position of the buffer giving a printf format
> > + * string. The contents will be shifted, not overwritten.
> > + */
> > +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
> > +		     va_list ap);
> > +
> > +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
> > +
> >  /**
> >   * Remove given amount of data from a given position of the buffer.
> >   */
> > -- 
> > 2.19.1.878.g0482332a22
> > 
>