diff mbox series

[FFmpeg-devel,v2,1/4] avutil/internal: add FF_ALLOC_TYPED_ARRAY_OR_GOTO & FF_ALLOCZ_TYPED_ARRAY_OR_GOTO

Message ID 1589212343-8334-1-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers
Series [FFmpeg-devel,v2,1/4] avutil/internal: add FF_ALLOC_TYPED_ARRAY_OR_GOTO & FF_ALLOCZ_TYPED_ARRAY_OR_GOTO |

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lance Wang May 11, 2020, 3:52 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

These are similar to the existing FF_ALLOC_ARRAY_OR_GOTO & FF_ALLOCZ_ARRAY_OR_GOTO,
but the elsize is calcuated by sizeof(*p)

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/internal.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Marton Balint May 11, 2020, 6:34 p.m. UTC | #1 
On Mon, 11 May 2020, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> These are similar to the existing FF_ALLOC_ARRAY_OR_GOTO & FF_ALLOCZ_ARRAY_OR_GOTO,
> but the elsize is calcuated by sizeof(*p)
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavutil/internal.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index 4acbcf5..1be9001 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -173,6 +173,24 @@
>     }\
> }
> 
> +#define FF_ALLOC_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\

This should not duplicate functionality of FF_ALLOC_ARRAY_OR_GOTO, but it 
should be defined as
  FF_ALLOC_ARRAY_OR_GOTO(ctx, p, nelem, sizeof(*(p)), label)

> +{\
> +    p = av_malloc_array(nelem, sizeof(*p));\
> +    if (!p) {\
> +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> +        goto label;\
> +    }\
> +}
> +
> +#define FF_ALLOCZ_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\

Same here

Regards,
Marton

> +{\
> +    p = av_mallocz_array(nelem, sizeof(*p));\
> +    if (!p) {\
> +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> +        goto label;\
> +    }\
> +}
> +
> #include "libm.h"
> 
> /**
> -- 
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang May 12, 2020, 12:04 a.m. UTC | #2 
On Mon, May 11, 2020 at 08:34:49PM +0200, Marton Balint wrote:
> 
> 
> On Mon, 11 May 2020, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > These are similar to the existing FF_ALLOC_ARRAY_OR_GOTO & FF_ALLOCZ_ARRAY_OR_GOTO,
> > but the elsize is calcuated by sizeof(*p)
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavutil/internal.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> > 
> > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > index 4acbcf5..1be9001 100644
> > --- a/libavutil/internal.h
> > +++ b/libavutil/internal.h
> > @@ -173,6 +173,24 @@
> >     }\
> > }
> > 
> > +#define FF_ALLOC_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> 
> This should not duplicate functionality of FF_ALLOC_ARRAY_OR_GOTO, but it
> should be defined as
>  FF_ALLOC_ARRAY_OR_GOTO(ctx, p, nelem, sizeof(*(p)), label)
> 
> > +{\
> > +    p = av_malloc_array(nelem, sizeof(*p));\
> > +    if (!p) {\
> > +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> > +        goto label;\
> > +    }\
> > +}
> > +
> > +#define FF_ALLOCZ_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> 
> Same here

OK, will fix it.

> 
> Regards,
> Marton
> 
> > +{\
> > +    p = av_mallocz_array(nelem, sizeof(*p));\
> > +    if (!p) {\
> > +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> > +        goto label;\
> > +    }\
> > +}
> > +
> > #include "libm.h"
> > 
> > /**
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George May 12, 2020, 9:49 a.m. UTC | #3 
lance.lmwang@gmail.com (12020-05-11):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> These are similar to the existing FF_ALLOC_ARRAY_OR_GOTO & FF_ALLOCZ_ARRAY_OR_GOTO,
> but the elsize is calcuated by sizeof(*p)
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/internal.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index 4acbcf5..1be9001 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -173,6 +173,24 @@
>      }\
>  }
>  
> +#define FF_ALLOC_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> +{\
> +    p = av_malloc_array(nelem, sizeof(*p));\
> +    if (!p) {\
> +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> +        goto label;\
> +    }\
> +}
> +
> +#define FF_ALLOCZ_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> +{\
> +    p = av_mallocz_array(nelem, sizeof(*p));\
> +    if (!p) {\
> +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> +        goto label;\
> +    }\
> +}
> +
>  #include "libm.h"
>  
>  /**

Please NO!

These functions have a terrible design, let us fix them before extending
them.

First design mistake: no error code. A helper function for testing
memory allocation failure where AVERROR(ENOMEM) does not appear is
absurd.

Second design mistake: printing a message. Return the error code, let
the caller print the error message.

Third design mistake: hard-coded use of goto.

Best design:

#define FF_ALLOC_ARRAY_OR_GOTO(p, nelem, elsize, ret, error)\
{\
    p = av_malloc_array(nelem, elsize);\
    if (!p) {\
        ret = AVERROR(ENOMEM); \
        error;
    }\
}

Regards,
Lance Wang May 12, 2020, 1:55 p.m. UTC | #4 
On Tue, May 12, 2020 at 11:49:01AM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-05-11):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > These are similar to the existing FF_ALLOC_ARRAY_OR_GOTO & FF_ALLOCZ_ARRAY_OR_GOTO,
> > but the elsize is calcuated by sizeof(*p)
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavutil/internal.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/libavutil/internal.h b/libavutil/internal.h
> > index 4acbcf5..1be9001 100644
> > --- a/libavutil/internal.h
> > +++ b/libavutil/internal.h
> > @@ -173,6 +173,24 @@
> >      }\
> >  }
> >  
> > +#define FF_ALLOC_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> > +{\
> > +    p = av_malloc_array(nelem, sizeof(*p));\
> > +    if (!p) {\
> > +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> > +        goto label;\
> > +    }\
> > +}
> > +
> > +#define FF_ALLOCZ_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
> > +{\
> > +    p = av_mallocz_array(nelem, sizeof(*p));\
> > +    if (!p) {\
> > +        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
> > +        goto label;\
> > +    }\
> > +}
> > +
> >  #include "libm.h"
> >  
> >  /**
> 
> Please NO!
> 
> These functions have a terrible design, let us fix them before extending
> them.
> 
> First design mistake: no error code. A helper function for testing
> memory allocation failure where AVERROR(ENOMEM) does not appear is
> absurd.
> 
> Second design mistake: printing a message. Return the error code, let
> the caller print the error message.
> 
> Third design mistake: hard-coded use of goto.
No, it's not hard-coded, you can name your own goto label.

So below is my change by your proposal:
 #define FF_ALLOC_ARRAY_OR_GOTO(p, nelem, elsize, ret, label)\
 {\
     p = av_malloc_array(nelem, elsize);\
     if (!p) {\
         ret = AVERROR(ENOMEM); \
         goto label;
     }\
 }


> 
> Best design:
> 
> #define FF_ALLOC_ARRAY_OR_GOTO(p, nelem, elsize, ret, error)\
> {\
>     p = av_malloc_array(nelem, elsize);\
>     if (!p) {\
>         ret = AVERROR(ENOMEM); \
>         error;
>     }\
> }
> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George May 12, 2020, 1:58 p.m. UTC | #5 
lance.lmwang@gmail.com (12020-05-12):
> No, it's not hard-coded, you can name your own goto label.

The label is not hard-coded. The goto is hard-coded. It should not be.
Frequently, a return would be enough.

Regards,
Marton Balint May 12, 2020, 2:06 p.m. UTC | #6 
On Tue, 12 May 2020, Nicolas George wrote:

> lance.lmwang@gmail.com (12020-05-12):
>> No, it's not hard-coded, you can name your own goto label.
>
> The label is not hard-coded. The goto is hard-coded. It should not be.
> Frequently, a return would be enough.


You mean this?

  #define FF_ALLOC_ARRAY_OR_ERROR(p, nelem, elsize, errstatement)\
  {\
      p = av_malloc_array(nelem, elsize);\
      if (!p) {\
          errstatement; \
      }\
  }

Regards,
Marton
Nicolas George May 12, 2020, 2:11 p.m. UTC | #7 
Marton Balint (12020-05-12):
> You mean this?
> 
>  #define FF_ALLOC_ARRAY_OR_ERROR(p, nelem, elsize, errstatement)\
>  {\
>      p = av_malloc_array(nelem, elsize);\
>      if (!p) {\
>          errstatement; \
>      }\
>  }

Exactly.

But I am rather in favor of making "something = AVERROR(ENOMEM)"
mandatory too:

#define FF_ALLOC_ARRAY_OR_ERROR(p, nelem, elsize, ret, errstatement)\
{\
    p = av_malloc_array(nelem, elsize);\
    if (!p) {\
        ret = AVERROR(ENOMEM); \
        errstatement; \
    }\
}

Regards,
Lance Wang May 12, 2020, 2:14 p.m. UTC | #8 
On Tue, May 12, 2020 at 03:58:24PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-05-12):
> > No, it's not hard-coded, you can name your own goto label.
> 
> The label is not hard-coded. The goto is hard-coded. It should not be.
> Frequently, a return would be enough.

return the error code directly? I recall some goto have extra cleanup function,
I'm not sure whether it's OK to return error code directly without goto.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang May 12, 2020, 2:19 p.m. UTC | #9 
On Tue, May 12, 2020 at 04:11:34PM +0200, Nicolas George wrote:
> Marton Balint (12020-05-12):
> > You mean this?
> > 
> >  #define FF_ALLOC_ARRAY_OR_ERROR(p, nelem, elsize, errstatement)\
> >  {\
> >      p = av_malloc_array(nelem, elsize);\
> >      if (!p) {\
> >          errstatement; \
> >      }\
> >  }
> 
> Exactly.
> 
> But I am rather in favor of making "something = AVERROR(ENOMEM)"
> mandatory too:
> 
> #define FF_ALLOC_ARRAY_OR_ERROR(p, nelem, elsize, ret, errstatement)\
> {\
>     p = av_malloc_array(nelem, elsize);\
>     if (!p) {\
>         ret = AVERROR(ENOMEM); \
>         errstatement; \
>     }\
> }

OK, it's more clear, I got it.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George May 12, 2020, 3:29 p.m. UTC | #10 
lance.lmwang@gmail.com (12020-05-12):
> return the error code directly? I recall some goto have extra cleanup function,
> I'm not sure whether it's OK to return error code directly without goto.

There are many places that do. Forcing them to use a goto just to be
able to benefit from that macro would be backwards.

Regards,
Lance Wang May 12, 2020, 3:33 p.m. UTC | #11 
On Tue, May 12, 2020 at 05:29:13PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-05-12):
> > return the error code directly? I recall some goto have extra cleanup function,
> > I'm not sure whether it's OK to return error code directly without goto.
> 
> There are many places that do. Forcing them to use a goto just to be
> able to benefit from that macro would be backwards.

OK, I'll submit a patch with the proposal for review.

> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavutil/internal.h b/libavutil/internal.h
index 4acbcf5..1be9001 100644
--- a/libavutil/internal.h
+++ b/libavutil/internal.h
@@ -173,6 +173,24 @@ 
     }\
 }
 
+#define FF_ALLOC_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
+{\
+    p = av_malloc_array(nelem, sizeof(*p));\
+    if (!p) {\
+        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
+        goto label;\
+    }\
+}
+
+#define FF_ALLOCZ_TYPED_ARRAY_OR_GOTO(ctx, p, nelem, label)\
+{\
+    p = av_mallocz_array(nelem, sizeof(*p));\
+    if (!p) {\
+        av_log(ctx, AV_LOG_ERROR, "Cannot allocate memory.\n");\
+        goto label;\
+    }\
+}
+
 #include "libm.h"
 
 /**