[FFmpeg-devel] [PATCH 3/8] avutil/mem: Add av_fast_realloc_array()

[Tomas Härdin]

ons 2022-09-28 klockan 13:06 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2022-09-27 klockan 17:23 +0200 skrev Tomas Härdin:
> > > mån 2022-09-26 klockan 16:24 +0200 skrev Tomas Härdin:
> > > > mån 2022-09-26 klockan 14:25 +0200 skrev Andreas Rheinhardt:
> > > > > Anton Khirnov:
> > > > > > Quoting Andreas Rheinhardt (2022-07-14 14:51:07)
> > > > > > > Anton Khirnov:
> > > > > > > > Quoting Andreas Rheinhardt (2022-07-12 16:12:16)
> > > > > > > > > Anton really dislikes the av_fast_* naming and
> > > > > > > > > instead
> > > > > > > > > wants
> > > > > > > > > this to be
> > > > > > > > > called av_realloc_array_reuse(). I don't care either
> > > > > > > > > way.
> > > > > > > > > Any
> > > > > > > > > more
> > > > > > > > > opinions on this (or on the patch itself)?
> > > > > > > > 
> > > > > > > > If people dislike _reuse(), I am open to other
> > > > > > > > reasonable
> > > > > > > > suggestions.
> > > > > > > > This 'fast' naming sucks because
> > > > > > > > - it tells you nothing about how this function is
> > > > > > > > "fast"
> > > > > > > > - it is added at the beginning rather than the end,
> > > > > > > > which
> > > > > > > > is
> > > > > > > >   against standard namespacing conventions
> > > > > > > > 
> > > > > > > 
> > > > > > > Isn't reusing the basic modus operandi for a reallocation
> > > > > > > function? So
> > > > > > > your suggested name doesn't seem to fit either.
> > > > > > 
> > > > > > Ordinary realloc just keeps the data, I wouldn't call that
> > > > > > "reuse"
> > > > > > since
> > > > > > it will often be a copy. This "fast" realloc OTOH reuses
> > > > > > the
> > > > > > actual
> > > > > > buffer, same as all the other "fast" mem.h functions.
> > > > > > 
> > > > > > But feel free to suggest another naming pattern if you can
> > > > > > think
> > > > > > of
> > > > > > one.
> > > > > > 
> > > > > 
> > > > > I see two differences between this function and ordinary
> > > > > realloc:
> > > > > It
> > > > > never shrinks the buffer and it overallocates. These two
> > > > > properties
> > > > > make
> > > > > it more likely that these functions can avoid copies more
> > > > > often
> > > > > than
> > > > > plain realloc (but in contrast to realloc, we can not grow
> > > > > the
> > > > > buffer
> > > > > in
> > > > > case there is free space after it), but it is nevertheless
> > > > > the
> > > > > same
> > > > > as
> > > > > realloc.
> > > > > 
> > > > > But I don't really care that much about the name and will
> > > > > therefore
> > > > > use
> > > > > your name as I can't come up with anything better.
> > > > > (Of course, I am still open to alternative suggestions.)
> > > > > 
> > > > > - Andreas
> > > > 
> > > > So this means av_realloc_array_reuse()? Eh, it works. I will
> > > > add a
> > > > function that also zeroes the newly allocated space, what
> > > > should we
> > > > call that? av_realloc_array_reusez()?
> > > > av_realloc_array_reuse_zerofill()?
> > > 
> > > Here's a draft patch that calls it av_reallocz_array_reuse().
> > > Needs a
> > > minor version bump of course
> > 
> > This makes me realize something: av_realloc_array_reuse() requires
> > that
> > *nb_allocated == 0 initially but this isn't specified in the
> > documentation. Patch attached relaxes this.
> > 
> 
> * @param[in,out] nb_allocated  Pointer to the number of elements of
> the
> array
> *                              `*ptr`. `*nb_allocated` is updated to
> the new
> *                              number of allocated elements.
> 
> If *ptr == NULL, then the number of elements of said array is (of
> course) zero, so *nb_allocated has to be set to that.

Keep in mind av_malloc(0) and av_realloc(ptr, 0) are both legal and
will allocate one byte. Seems it's not a problem in this case luckily

> (But if you think
> that this needs to be said explicitly, I can document it.)

It's best to be explicit IMO, so that callers know they can't just use
an uninitialized size_t on the stack or something.

/Tomas