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@xxxxxx>
> > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx>
> > ---
> > 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
> >
>
