[FFmpeg-devel] [PATCH] Remove only use of compound literals in FFmpeg.

[Reimar Döffinger]

On Mon, Dec 30, 2013 at 11:00:52AM +0100, Reimar Döffinger wrote:
> On Mon, Dec 30, 2013 at 09:00:13AM +0100, Clément Bœsch wrote:
> > On Mon, Dec 30, 2013 at 08:28:13AM +0100, Reimar Döffinger wrote:
> > > On 30.12.2013, at 07:31, Clément Bœsch <u at pkh.me> wrote:
> > > > On Mon, Dec 30, 2013 at 01:09:17AM +0100, Nicolas George wrote:
> > > >> Le decadi 10 nivôse, an CCXXII, Reimar Döffinger a écrit :
> > > >>> However it still seems to be the only place where we use it?
> > > >> 
> > > >> I am afraid not: there is also the av_err2str() magic macro (in
> > > >> lavu/error.h), which is, if I say so myself, really convenient to use
> > > >> instead of the normal functions.
> > > >> 
> > > >> I suppose you could implement it using a static array, at the cost of 64
> > > >> extra bytes per use.
> > > 
> > > It can always be implemented as a local array with no disadvantage, it never does more than avoiding a local variable.
> > 
> > having a char buf[INCONSISTENT_SIZE] on top of a large function for an
> > label error at the end is a bit troublesome. Of course you can do it, but
> >     fprintf(stderr, "Error [%s]\n", av_err2str(ret));
> > is more cute than
> >     char buf[128];
> >     fprintf(stderr, "Error [%s]\n", av_make_error_string(buf, sizeof(buf), ret));
> 
> Sorry, I missed it was a function, not a macro.
> 
> > > In many cases it does less, since many compilers are unable to place them in .rodata and always build them on stack.
> > > 
> > > > Same goes for av_ts2str().
> > > 
> > > Is that function unused? I didn't notice it causing any issue.
> > 
> > Yes, in many place, and often in the same function call. To change that
> > you would need to declare like 4 or more "char bufN[64]", which sounds
> > like an expensive price for supporting a compiler no one uses anymore.
> 
> Oh, no doubt about that (though "no one" isn't _quite_ true, for better
> or worse). While I haven't tested if it actually works, that code however
> compiles just fine...
> So there is something special about that aresample code.
> Note that one of the reasons I investigate it is because I am always
> _very_ suspicious of any language feature that is only used in one
> single file in a whole project. Most of the time IMHO that means either
> it should be used in other places as well, or it's not a good idea to be
> used in that one file either.
> However at this point I seem to have made the wrong conclusions on what
> that feature actually is...

Ok, it seems that gcc 2.95 treats these like static initializers, all
elements of a compound literal need to be constant.
That is why all other cases work, this one is the only one with
non-constant initialization values.
That leaves the question if you think this patch is reasonable.
My personal opinion is that using this feature when it doesn't really
help much if any in readability isn't a good idea anyway, but before
I start a flame-war I don't mind dropping any of these patches.