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

[Ganesh Ajjanagadde]

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? I consider it ridiculous to add
a comment when the name tells what it does better and more succinctly.
We could add both, but I do not want to do that as generally FFmpeg
favors a slightly terse style common to many  C projects. Thus, if
adding the width, it should be to the function name, not to a comment.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel