[FFmpeg-cvslog] r19742 - trunk/libavutil/internal.h

[Benoit Fouet]

On 2009-09-03 17:27, Ramiro Polla wrote:
> On Wed, Sep 2, 2009 at 10:05 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
>> On Wed, Sep 02, 2009 at 03:32:26PM +0100, M?ns Rullg?rd wrote:
>>> Michael Niedermayer <michaelni at gmx.at> writes:
>>>
>>>> On Tue, Sep 01, 2009 at 04:16:36PM +0100, M?ns Rullg?rd wrote:
>>>>> Benoit Fouet <benoit.fouet at free.fr> writes:
>>>>>
>>>>>> On 2009-08-30 00:44, M?ns Rullg?rd wrote:
>>>>>>> ramiro <subversion at mplayerhq.hu> writes:
>>>>>>>
>>>>>>>> Author: ramiro
>>>>>>>> Date: Sun Aug 30 00:38:48 2009
>>>>>>>> New Revision: 19742
>>>>>>>>
>>>>>>>> Log:
>>>>>>>> Add CHECKED_ALLOC macro.
>>>>>>>> It works the same as CHECKED_ALLOCZ except that it does not
>>>>>>>> zero the allocated memory.
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>    trunk/libavutil/internal.h
>>>>>>>>
>>>>>>>> Modified: trunk/libavutil/internal.h
>>>>>>>> ==============================================================================
>>>>>>>> --- trunk/libavutil/internal.h Sat Aug 29 23:04:18 2009        (r19741)
>>>>>>>> +++ trunk/libavutil/internal.h Sun Aug 30 00:38:48 2009        (r19742)
>>>>>>>> @@ -249,6 +249,15 @@ if((y)<(x)){\
>>>>>>>>  #define perror please_use_av_log_instead_of_perror
>>>>>>>>  #endif
>>>>>>>>
>>>>>>>> +#define CHECKED_ALLOC(p, size)\
>>>>>>>> +{\
>>>>>>>> +    p= av_malloc(size);\
>>>>>>>> +    if(p==NULL && (size)!=0){\
>>>>>>>> +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate memory.");\
>>>>>>>> +        goto fail;\
>>>>>>>> +    }\
>>>>>>>> +}
>>>>>>> Looks like I missed some discussions...  This should be wrapped in
>>>>>>> do { } while(0) so if (foo) CHECKED_ALLOC(); else blah; can work.  It
>>>>>>> would also be nice if the label to goto were an argument.  As it is,
>>>>>>> you can only have one target per function that uses the macro.
>>>>>>> Finally, it could do with some beautifying, but that's secondary.
>>>>>>>
>>>>>> it would be cool to be able to pass an avctx too:
>>>>>>
>>>>>> #define CHECKED_ALLOC(ctx, p, size, label)                      \
>>>>>> do {                                                            \
>>>>>>     p = malloc(size);                                           \
>>>>>>     if (!p && (size)){                                          \
>>>>>>         av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n"); \
>>>>>>         goto label;                                             \
>>>>>>     }                                                           \
>>>>>> } while (0)
>>>>> While we're at it, shouldn't this have an FF_ prefix like other
>>>>> macros?
>>>> and i think
>>>> FF_ALLOC_OR_GOTO_FAIL(ctx, p, size)
>>>> is best
>>> Do you have a strong objection against passing the label as a
>>> parameter?
>> no, it just feels unneeded because i cant think of any case where we
>> would use it, all cases i can think of have a single "fail" target
>> that frees resources
> 
> Unneeded or not, it's just one more character if you do
> FF_ALLOC_OR_GOTO(ctx, p, size, fail)
> instead of
> FF_ALLOC_OR_GOTO_FAIL(ctx, p, size)
> 
> and I'm not a big fan of upper case =)
> 
> Patches attached. In swscale where there is no context I left it
> without context and will send a patch later.
> 
> In what order should this be applied? One depends on the other and
> either will break when the other is applied first.
> 

> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index 14ea95b..7b665a9 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -249,21 +249,21 @@ if ((y) < (x)) {\
>  #define perror please_use_av_log_instead_of_perror
>  #endif
>
> -#define CHECKED_ALLOC(p, size)\
> +#define FF_ALLOC_OR_GOTO(ctx, p, size, label)\
>  {\
>      p = av_malloc(size);\
>      if (p == NULL && (size) != 0) {\
> -        av_log(NULL, AV_LOG_ERROR, "Cannot allocate memory.");\
> -        goto fail;\
> +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.");\
> +        goto label;\
>

not related to this change, but doesn't this also miss a '\n' ?

Ben