[FFmpeg-devel] [PATCH] intmath: remove av_ctz.

[Michael Niedermayer]

On Sun, Oct 11, 2015 at 10:02:29PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Sun, Oct 11, 2015 at 9:17 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> wrote:
> 
> > On Sun, Oct 11, 2015 at 9:12 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
> > wrote:
> > > On Sun, Oct 11, 2015 at 6:04 PM, Ronald S. Bultje <rsbultje at gmail.com>
> > wrote:
> > >> Hi,
> > >>
> > >> On Sun, Oct 11, 2015 at 5:52 PM, Andreas Cadhalpun <
> > >> andreas.cadhalpun at googlemail.com> wrote:
> > >>
> > >>> On 11.10.2015 23:44, Ronald S. Bultje wrote:
> > >>> > It's a non-installed header and only used in one place (flacenc).
> > >>> > Since ff_ctz is static inline, it's fine to use that instead.
> > >>> > ---
> > >>> >  doc/APIchanges       |  3 ---
> > >>> >  libavcodec/flacenc.c |  2 +-
> > >>> >  libavutil/intmath.c  |  5 -----
> > >>> >  libavutil/intmath.h  | 14 ++++++--------
> > >>> >  4 files changed, 7 insertions(+), 17 deletions(-)
> > >>>
> > >>> Should be fine.
> > >>
> > >>
> > >> Thanks, pushed.
> > >
> > > Since there is still time, and I did not think of this before, I would
> > > like to replace ff_ctz with ff_ctz32. There are a couple of reasons:
> > > 1. It is used with an int32 argument in flacenc.
> > > 2. I can do a deBruijn optimization for this as well. With an int also
> > > I could do it, but it would need some ifdefry depending on whether int
> > > is 32 bit or 64 bits.
> > >
> > > Let me see if I understand API/ABI with respect to this proposed
> > > change correctly now:
> > > API is not broken, as this is not public.
> > > ABI is broken, since the width of operands to ff_ctz has could change
> > > from 64 to 32 bits.
> >
> > That actually raises the question of whether int = 32 bits is assumed
> > everywhere. For instance, if int = 64 bits, at least on the Intel
> > compiler, ff_ctz is broken: AFAIK, _bit_scan_forward operates on 32
> > bit operands.
> 
> 
> I think you can safely assume int is 32bit, we do so in numerous places.

we assume int >= 32 bit all over (posix gurantees it)
int == 32bit is assumed in some arch specific code like simd asm

generic code should not assume int to be 32bit
or rather IMHO we should not conciously add any such code as it just
violates C and will break on some (currently) uncommon platforms/configs

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151012/69bf9fdf/attachment.sig>