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

[Clément Bœsch]

On Mon, Dec 30, 2013 at 11:07:42AM +0100, Reimar Döffinger wrote:
> 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.

I don't mind the af_aresample.c change (but I'm not a maintainer of that
filter), I don't think it hurts, I was just a bit reluctant about the
changes with av_{err,ts}2str()

Thought, it's strange that it only complains in that place:

[~/src/ffmpeg]☭ git grep '(int\[\])'
libavcodec/ac3enc.c:            num_blocks = ((int[]){ 1, 2, 3, 6 })[num_blks_code];
libavcodec/flacenc.c:    s->options.block_time_ms = ((int[]){ 27, 27, 27,105,105,105,105,105,105,105,105,105,105
libavcodec/flacenc.c:        s->options.lpc_type  = ((int[]){ FF_LPC_TYPE_FIXED,    FF_LPC_TYPE_FIXED,    FF_LPC
libavcodec/flacenc.c:    s->options.min_prediction_order = ((int[]){  2,  0,  0,  1,  1,  1,  1,  1,  1,  1,  1,
libavcodec/flacenc.c:    s->options.max_prediction_order = ((int[]){  3,  4,  4,  6,  8,  8,  8,  8, 12, 12, 12,
libavcodec/flacenc.c:        s->options.prediction_order_method = ((int[]){ ORDER_METHOD_EST,    ORDER_METHOD_ES
libavcodec/flacenc.c:        s->options.min_partition_order = ((int[]){  2,  2,  0,  0,  0,  0,  0,  0,  0,  0, 
libavcodec/flacenc.c:        s->options.max_partition_order = ((int[]){  2,  2,  3,  3,  3,  8,  8,  8,  8,  8, 
libavcodec/g726.c:    avctx->frame_size = ((int[]){ 4096, 2736, 2048, 1640 })[c->code_size - 2];
libavcodec/tiffenc.c:    flip ^= ((int[]) { 0, 0, 0, 1, 3, 3 })[type];
libavfilter/af_aresample.c:        out_samplerates = ff_make_format_list((int[]){ out_rate, -1 });
libavfilter/af_aresample.c:        out_formats = ff_make_format_list((int[]){ out_format, -1 });
libavformat/mov.c:    st->codec->channels = ((int[]){2,1,2,3,3,4,4,5})[acmod] + lfeon;

> 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.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131230/c3907083/attachment.asc>