On Wed, Oct 14, 2015 at 8:34 AM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > 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? > > > 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. ;-).
Got it, useful tip that I had forgotten. Patchv2 posted. > > Ronald > _______________________________________________ > 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
