Message ID | 1589212343-8334-1-git-send-email-lance.lmwang@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel,v2,1/4] avutil/internal: add FF_ALLOC_TYPED_ARRAY_OR_GOTO & FF_ALLOCZ_TYPED_ARRAY_OR_GOTO | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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".
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".
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,
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
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,
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
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,
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".
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".
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,
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 --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" /**