[flac-dev] PATCH: bugfixes for bitmath.h

[lvqcl]

More or less detailed explanation of this patch.


1. The first parameter of _BitScanReverse() and _BitScanReverse64() is a pointer to unsigned long (4-byte int). However _BitScanReverse64() is called with a pointer to FLAC__uint64 (8-byte int). IMHO it's a bug and this patch changes the type of idx variable inside FLAC__bitmath_ilog2_wide() from FLAC__uint64 to unsigned long.

The type of idx variable inside FLAC__bitmath_ilog2() was also changed (from FLAC__uint32 to unsigned long), for symmetry.




2. FLAC__clz_uint32() returns the result of BitScanReverse() XOR 31. FLAC__bitmath_ilog2() returns 31-FLAC__clz_uint32() == 31-(BSR^31).

As 0<=BSR<=31 so 31-(BSR^31) == BSR. But I don't think that compilers are so smart that can optimize this expression. So it is better to change FLAC__bitmath_ilog2() so it simply calls BitScanReverse() when possible.



3. FLAC__bitmath_ilog2_wide() cannot be compiled with MSVC: it has 'if (v == 0)...' statement followed by a variable (either idx or DEBRUIJN_IDX64) definition. But all calls of this function have FLAC__ASSERT statements that ensure that its argument is greater than 0. So this 'if()' can be removed.



4. Am I right that FLAC__bitmath_ilog2_wide() and FLAC__bitmath_ilog2() return the same value if their argument is less than 2^32? Then FLAC__bitmath_ilog2_wide() works correctly only when compiled with GCC: it should return idx, not idx^63U. Also the result of de Bruijn sequences is off by 1.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bitmath.patch
Type: application/octet-stream
Size: 2625 bytes
Desc: not available
Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20130904/9457509c/attachment.obj