On Mon, Oct 12, 2015 at 7:14 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Mon, Oct 12, 2015 at 12:37 AM, James Almer <jamr...@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.
What is the objection to the patch as it stands? IIRC Michael said he does not like bit width specific stuff (in particular int = 32 bits), to which I replied saying that the old code anyway made that assumption on non GCC platforms. Thus, I argued that this patch represents a strict improvement over existing code. If Michael (or others) want a proper non "int = 32" function, that is a separate concern which I can address in a follow-up patch. > >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel