[FFmpeg-devel] [PATCH] avutil/intmath: use de Bruijn based ff_ctz

[Ronald S. Bultje]

Hi,

On Mon, Oct 12, 2015 at 7:14 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
wrote:

> On Mon, Oct 12, 2015 at 12:37 AM, James Almer <jamrial at gmail.com> wrote:
> > On 10/11/2015 10:55 PM, Ganesh Ajjanagadde wrote:
> >> It has already been demonstrated that the de Bruijn method has benefits
> >> over the current implementation: commit
> 971d12b7f9d7be3ca8eb98e6c04ed521f83cbd3c.
> >> That commit implemented it for long long, this extends it to the 32 bit
> >> version.
> >>
> >> The function is renamed from ff_ctz to ff_ctz32 since it crucially
> >> depends on the 32 bit width of its argument. This is not an issue, as
> the
> >> only usage in avcodec/flacenc uses an int32_t anyway.
> >
> > I personally don't think the renaming is needed, for that matter. The
> > function takes an int as argument, and as far as ffmpeg supported arches
> > go those are 32 bits.
> > If you really want to be sure, just add a comment that the argument
> > absolutely needs to be 32 bits and that should be enough.
>
> This I don't understand. Why not make the function self documenting
> when we achieve it with zero penalty?


>From what I can see, it's mostly a case of your patch doing two things:
- it renames the function
- it reimplements it
Where possible, we prefer patches that do exactly one thing. I'd
reimplement the function and have that go in as-is, and then renaming can
become the eternal bikeshed that it always becomes. See, these things
aren't technical in nature - unlike your reimplementation, which is simply
faster because it's a better algorithm. These things are just based on
personal preference, and so you'll never get a clean vote.

So, statistically speaking, if you want your patch to have the highest
chance of going in, make it do the minimal amount of change relative to
status quo, and make it do so in as little code change as possible. ;-).

Ronald