[FFmpeg-devel] [PATCH 1/3] avformat/avio: add avio_print_n_strings and avio_print

Reimar Döffinger Reimar.Doeffinger at gmx.de
Sun Aug 11 23:03:37 EEST 2019


On 11.08.2019, at 21:51, Marton Balint <cus at passwd.hu> wrote:

> 
> 
> On Sun, 11 Aug 2019, Reimar Döffinger wrote:
> 
>> On 11.08.2019, at 20:41, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>> 
>>> On 05.08.2019, at 23:34, Marton Balint <cus at passwd.hu> wrote:
>>>> These functions can be used to print a variable number of strings consecutively
>>>> to the IO context. Unlike av_bprintf, no temporery buffer is necessary.
>>> Hm, is there a use-example patch I missed?
>> 
>> Sorry, I see it now, and also the argument about the macro providing type checking.
>> Ignore me if you feel the code is fine as-is.
>> 
>>> Also is the #define ugliness worth it compared to just requiring NULL termination?
>>> You can use the sentinel function attribute to have the compiler check that users do not forget it.
> 
> There is yet another approach which does not count parameters but uses a NULL sentinel, and is more simple:
> 
> void avio_print_string_array(AVIOContext *s, const char *strings[])
> {
>    for(; strings[0]; strings++)
>        avio_write(s, (const unsigned char *)strings[0], strlen(strings[0]));
> }

Nitpick: I find using [0] confusing when used on a loop variable and would consider *strings better

> #define avio_print(s, ...) \
>    avio_print_string_array(s, (const char*[]){__VA_ARGS__, NULL})
> 
> This might also has the benefit that __VA_ARGS__ is referenced only once, because for the old version some compilers (msvc) was not optimizing away the additional local variable and their string references which were used in the sizeof magic.
> 
> I can change the patch to use this version if people prefer.

It also avoids potential multiple-evaluation issues, so it does seem more robust to me.
But I admit my macro-foo is rather weak due to too much C++ use in the last years...


More information about the ffmpeg-devel mailing list