On Sun, Aug 23, 2015 at 9:57 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Sun, Aug 23, 2015 at 4:20 AM, Nicolas George <geo...@nsup.org> wrote: >> Le quintidi 5 fructidor, an CCXXIII, Ganesh Ajjanagadde a écrit : >>> >> + return abs((int) (t1 - t2)) + abs((int) ((c1 & 0x000000ff) - (c2 & >>> >> 0x000000ff))) + >>> >> + abs((int) (((c1 & 0x0000ff00) >> 8) - ((c2 & 0x0000ff00) >> >>> >> 8))) + >>> >> + abs((int) (((c1 & 0x00ff0000) >> 16) - ((c2 & 0x00ff0000) >> >>> >> 16))); >>> The cast idea is incorrect as stated, though your idea is sound: >>> int is not guaranteed to be of 32 bits on all platforms. >>> Casting a uint32_t to an int will be bad. >> >> Look at the code itself: the numbers are all smaller than 768. The cast in >> that case is completely reliable. > > You are right, in current use cases your solution would work. > However, it lacks generality and is prone to mistakes: > the code writer must reason with the bit widths as you did.
Actually, regardless this needs to be done, since abs is for ints. Nevertheless, I think the second macro is better (without the cast to ints), since it correctly preserves the ranges for which operation is allowed. > Do you also agree that a macro is a better idea? > >> >>> I assume this is what you had in mind with your macro idea. >> >> I had in mind just a local macro to make that precise part of the code more >> readable. > > Such a macro would be duplicated across files. > I just wanted to place it near the FFABS definition for removing such > duplication, > unless people object to this. > >> >> Regards, >> >> -- >> Nicolas George >> >> _______________________________________________ >> 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
