diff mbox series

[FFmpeg-devel,1/2] avutil/common: Add FFINCREASE_PTR()

Message ID 20210219204345.14218-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avutil/common: Add FFINCREASE_PTR()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer Feb. 19, 2021, 8:43 p.m. UTC
Suggested-by: Andreas Rheinhardt
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 doc/APIchanges     | 3 +++
 libavutil/common.h | 2 ++
 2 files changed, 5 insertions(+)

Comments

Andreas Rheinhardt Feb. 19, 2021, 9 p.m. UTC | #1
Michael Niedermayer:
> Suggested-by: Andreas Rheinhardt
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  doc/APIchanges     | 3 +++
>  libavutil/common.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index c353d2d281..e38a5cb91c 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
> +  Add FFINCREASE_PTR()
> +
>  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
>    Deprecated avdevice_capabilities_create() and
>    avdevice_capabilities_free().
> diff --git a/libavutil/common.h b/libavutil/common.h
> index aee353d399..bf35bc8507 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -108,6 +108,8 @@
>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>  
> +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
> +
>  /* misc math functions */
>  
>  #ifdef HAVE_AV_CONFIG_H
> 
If this is intended to be a public macro, it should have a proper AV prefix.

- Andreas
Nicolas George Feb. 19, 2021, 9:28 p.m. UTC | #2
Michael Niedermayer (12021-02-19):
> Suggested-by: Andreas Rheinhardt
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  doc/APIchanges     | 3 +++
>  libavutil/common.h | 2 ++
>  2 files changed, 5 insertions(+)

Is it only for NULL+0 or is it for all NULL+x?

It is ok to hide NULL+0, but NULL+x is a different kind of bug, more
serious, but the macro hides it. Maybe add an av_assert2()?

Regards,
Andreas Rheinhardt Feb. 19, 2021, 9:34 p.m. UTC | #3
Nicolas George:
> Michael Niedermayer (12021-02-19):
>> Suggested-by: Andreas Rheinhardt
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  doc/APIchanges     | 3 +++
>>  libavutil/common.h | 2 ++
>>  2 files changed, 5 insertions(+)
> 
> Is it only for NULL+0 or is it for all NULL+x?
> 
> It is ok to hide NULL+0, but NULL+x is a different kind of bug, more
> serious, but the macro hides it. Maybe add an av_assert2()?
> 
It is only for NULL+0; NULL+x is a real bug after all.
And the macro doesn't hide it; NULL+x can still be detected with all the
typical tools (ubsan) to detect NULL+x. Doing it this way has the
advantage of allowing the compiler to optimize the branch away (GCC does
it).

- Andreas
Nicolas George Feb. 19, 2021, 9:54 p.m. UTC | #4
Andreas Rheinhardt (12021-02-19):
> It is only for NULL+0; NULL+x is a real bug after all.
> And the macro doesn't hide it; NULL+x can still be detected with all the
> typical tools (ubsan) to detect NULL+x. Doing it this way has the
> advantage of allowing the compiler to optimize the branch away (GCC does
> it).

Oh, I had read the macro the wrong way. This version is cleverer.

Regards,
Michael Niedermayer March 13, 2021, 3:18 p.m. UTC | #5
On Fri, Feb 19, 2021 at 10:00:39PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Suggested-by: Andreas Rheinhardt
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  doc/APIchanges     | 3 +++
> >  libavutil/common.h | 2 ++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index c353d2d281..e38a5cb91c 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >  
> >  API changes, most recent first:
> >  
> > +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
> > +  Add FFINCREASE_PTR()
> > +
> >  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
> >    Deprecated avdevice_capabilities_create() and
> >    avdevice_capabilities_free().
> > diff --git a/libavutil/common.h b/libavutil/common.h
> > index aee353d399..bf35bc8507 100644
> > --- a/libavutil/common.h
> > +++ b/libavutil/common.h
> > @@ -108,6 +108,8 @@
> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> >  
> > +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
> > +
> >  /* misc math functions */
> >  
> >  #ifdef HAVE_AV_CONFIG_H
> > 
> If this is intended to be a public macro, it should have a proper AV prefix.

will apply with AV prefix

thx

[...]
Lynne March 13, 2021, 3:41 p.m. UTC | #6
Mar 13, 2021, 16:18 by michael@niedermayer.cc:

> On Fri, Feb 19, 2021 at 10:00:39PM +0100, Andreas Rheinhardt wrote:
>
>> Michael Niedermayer:
>> > Suggested-by: Andreas Rheinhardt
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  doc/APIchanges     | 3 +++
>> >  libavutil/common.h | 2 ++
>> >  2 files changed, 5 insertions(+)
>> > 
>> > diff --git a/doc/APIchanges b/doc/APIchanges
>> > index c353d2d281..e38a5cb91c 100644
>> > --- a/doc/APIchanges
>> > +++ b/doc/APIchanges
>> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>> > 
>> >  API changes, most recent first:
>> > 
>> > +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
>> > +  Add FFINCREASE_PTR()
>> > +
>> >  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
>> >    Deprecated avdevice_capabilities_create() and
>> >    avdevice_capabilities_free().
>> > diff --git a/libavutil/common.h b/libavutil/common.h
>> > index aee353d399..bf35bc8507 100644
>> > --- a/libavutil/common.h
>> > +++ b/libavutil/common.h
>> > @@ -108,6 +108,8 @@
>> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
>> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
>> > 
>> > +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
>> > +
>> >  /* misc math functions */
>> > 
>> >  #ifdef HAVE_AV_CONFIG_H
>> > 
>> If this is intended to be a public macro, it should have a proper AV prefix.
>>
>
> will apply with AV prefix
>

Could we not make it public? Macros like FF_ARRAY_ELEMS aren't,
and I think this is one of those utility functions that API users probably
won't find useful.
Plus the name is kinda long.
Michael Niedermayer March 13, 2021, 3:55 p.m. UTC | #7
On Sat, Mar 13, 2021 at 04:41:39PM +0100, Lynne wrote:
> Mar 13, 2021, 16:18 by michael@niedermayer.cc:
> 
> > On Fri, Feb 19, 2021 at 10:00:39PM +0100, Andreas Rheinhardt wrote:
> >
> >> Michael Niedermayer:
> >> > Suggested-by: Andreas Rheinhardt
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  doc/APIchanges     | 3 +++
> >> >  libavutil/common.h | 2 ++
> >> >  2 files changed, 5 insertions(+)
> >> > 
> >> > diff --git a/doc/APIchanges b/doc/APIchanges
> >> > index c353d2d281..e38a5cb91c 100644
> >> > --- a/doc/APIchanges
> >> > +++ b/doc/APIchanges
> >> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >> > 
> >> >  API changes, most recent first:
> >> > 
> >> > +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
> >> > +  Add FFINCREASE_PTR()
> >> > +
> >> >  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
> >> >    Deprecated avdevice_capabilities_create() and
> >> >    avdevice_capabilities_free().
> >> > diff --git a/libavutil/common.h b/libavutil/common.h
> >> > index aee353d399..bf35bc8507 100644
> >> > --- a/libavutil/common.h
> >> > +++ b/libavutil/common.h
> >> > @@ -108,6 +108,8 @@
> >> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> >> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> >> > 
> >> > +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
> >> > +
> >> >  /* misc math functions */
> >> > 
> >> >  #ifdef HAVE_AV_CONFIG_H
> >> > 
> >> If this is intended to be a public macro, it should have a proper AV prefix.
> >>
> >
> > will apply with AV prefix
> >
> 
> Could we not make it public? Macros like FF_ARRAY_ELEMS aren't,
> and I think this is one of those utility functions that API users probably
> won't find useful.
> Plus the name is kinda long.

sure, what do others prefer, iam fine with either

thx

[...]
Michael Niedermayer March 13, 2021, 9:20 p.m. UTC | #8
On Sat, Mar 13, 2021 at 04:41:39PM +0100, Lynne wrote:
> Mar 13, 2021, 16:18 by michael@niedermayer.cc:
> 
> > On Fri, Feb 19, 2021 at 10:00:39PM +0100, Andreas Rheinhardt wrote:
> >
> >> Michael Niedermayer:
> >> > Suggested-by: Andreas Rheinhardt
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  doc/APIchanges     | 3 +++
> >> >  libavutil/common.h | 2 ++
> >> >  2 files changed, 5 insertions(+)
> >> > 
> >> > diff --git a/doc/APIchanges b/doc/APIchanges
> >> > index c353d2d281..e38a5cb91c 100644
> >> > --- a/doc/APIchanges
> >> > +++ b/doc/APIchanges
> >> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >> > 
> >> >  API changes, most recent first:
> >> > 
> >> > +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
> >> > +  Add FFINCREASE_PTR()
> >> > +
> >> >  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
> >> >    Deprecated avdevice_capabilities_create() and
> >> >    avdevice_capabilities_free().
> >> > diff --git a/libavutil/common.h b/libavutil/common.h
> >> > index aee353d399..bf35bc8507 100644
> >> > --- a/libavutil/common.h
> >> > +++ b/libavutil/common.h
> >> > @@ -108,6 +108,8 @@
> >> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> >> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> >> > 
> >> > +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
> >> > +
> >> >  /* misc math functions */
> >> > 
> >> >  #ifdef HAVE_AV_CONFIG_H
> >> > 
> >> If this is intended to be a public macro, it should have a proper AV prefix.
> >>
> >
> > will apply with AV prefix
> >
> 
> Could we not make it public? Macros like FF_ARRAY_ELEMS aren't,
> and I think this is one of those utility functions that API users probably
> won't find useful.

> Plus the name is kinda long.

would you prefer FF_PTR_ADD() ?
or do you have another name in mind ?

thx

[..]
Michael Niedermayer March 14, 2021, 11:01 p.m. UTC | #9
On Sat, Mar 13, 2021 at 10:20:37PM +0100, Michael Niedermayer wrote:
> On Sat, Mar 13, 2021 at 04:41:39PM +0100, Lynne wrote:
> > Mar 13, 2021, 16:18 by michael@niedermayer.cc:
> > 
> > > On Fri, Feb 19, 2021 at 10:00:39PM +0100, Andreas Rheinhardt wrote:
> > >
> > >> Michael Niedermayer:
> > >> > Suggested-by: Andreas Rheinhardt
> > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >> > ---
> > >> >  doc/APIchanges     | 3 +++
> > >> >  libavutil/common.h | 2 ++
> > >> >  2 files changed, 5 insertions(+)
> > >> > 
> > >> > diff --git a/doc/APIchanges b/doc/APIchanges
> > >> > index c353d2d281..e38a5cb91c 100644
> > >> > --- a/doc/APIchanges
> > >> > +++ b/doc/APIchanges
> > >> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> > >> > 
> > >> >  API changes, most recent first:
> > >> > 
> > >> > +2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
> > >> > +  Add FFINCREASE_PTR()
> > >> > +
> > >> >  2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
> > >> >    Deprecated avdevice_capabilities_create() and
> > >> >    avdevice_capabilities_free().
> > >> > diff --git a/libavutil/common.h b/libavutil/common.h
> > >> > index aee353d399..bf35bc8507 100644
> > >> > --- a/libavutil/common.h
> > >> > +++ b/libavutil/common.h
> > >> > @@ -108,6 +108,8 @@
> > >> >  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
> > >> >  #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
> > >> > 
> > >> > +#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
> > >> > +
> > >> >  /* misc math functions */
> > >> > 
> > >> >  #ifdef HAVE_AV_CONFIG_H
> > >> > 
> > >> If this is intended to be a public macro, it should have a proper AV prefix.
> > >>
> > >
> > > will apply with AV prefix
> > >
> > 
> > Could we not make it public? Macros like FF_ARRAY_ELEMS aren't,
> > and I think this is one of those utility functions that API users probably
> > won't find useful.
> 
> > Plus the name is kinda long.
> 
> would you prefer FF_PTR_ADD() ?

will submit a patch with that and without adding it to APIchanges



[...]
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index c353d2d281..e38a5cb91c 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2021-02-xx - xxxxxxxxxx - lavu 57.xx.100 - common.h
+  Add FFINCREASE_PTR()
+
 2021-02-14 - xxxxxxxxxx - lavd 58.12.100 - avdevice.h
   Deprecated avdevice_capabilities_create() and
   avdevice_capabilities_free().
diff --git a/libavutil/common.h b/libavutil/common.h
index aee353d399..bf35bc8507 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -108,6 +108,8 @@ 
 #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 
+#define FFINCREASE_PTR(ptr, off) ((off) ? (ptr) + (off) : (ptr))
+
 /* misc math functions */
 
 #ifdef HAVE_AV_CONFIG_H