[FFmpeg-devel] [PATCH] avcodec: improve the function of FF_ALLOC{Z}{_ARRAY}_OR_GOTO

[Marton Balint]

On Tue, 12 May 2020, Nicolas George wrote:

> Marton Balint (12020-05-12):
>>> -    FF_ALLOCZ_ARRAY_OR_GOTO(avctx, s->buffer.samples, s->channels, 3 * 1024 * sizeof(s->buffer.samples[0]), alloc_fail);
>>> -    FF_ALLOCZ_ARRAY_OR_GOTO(avctx, s->cpe, s->chan_map[0], sizeof(ChannelElement), alloc_fail);
>>> +    int ret, ch;
>>> +    FF_ALLOCZ_ARRAY_OR_GOTO(s->buffer.samples, s->channels, 3 * 1024 * sizeof(s->buffer.samples[0]), ret, return ret);
>>> +    FF_ALLOCZ_ARRAY_OR_GOTO(s->cpe, s->chan_map[0], sizeof(ChannelElement), ret, return ret);
>>
>> If you want to change the existing macro, then you have to rename it,
>> because it no longer does goto...
>
> Indeed. But also, it must do the same. Replacing a goto with a direct
> return is not correct.
>
>> Also I kind of disagree with Nicholas that we should always assign a ret
>> variable, I think a single error statement is better. For example in the
>> case above a goto fail is a lot nicer (and returning ENOMEM) there.
>
> This is a mistake because you invent the error code on the call site.
> The function/macro knows what the error code, the caller uses the error
> code. Otherwise, changes in the function/macro can leave the error code
> invalid.
>
> We should never let the caller assume the error code.

And you assume that I want to assign the error code to ret. Wrong. What if 
I want to return it as is? Or what if I want to return NULL beacuse the 
function returns a pointer? Using variables is complicated. Constants 
make the code more simple and readable.

Regards,
Marton