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

[Erik de Castro Lopo]

lvqcl wrote:

> More or less detailed explanation of this patch.

I've applied part of that patch.

> 1. The first parameter of _BitScanReverse() and _BitScanReverse64() is
> a pointer to unsigned long (4-byte int).

On windows, yes, unsigned long is 4 bytes for both 32 and 64 bit versions.
This is not true for most Unices.

> However _BitScanReverse64() is called with a pointer to FLAC__uint64
> (8-byte int). IMHO it's a bug

I would not call that a bug, just a result of trying to write cross
platform code and relying on functions that are not specified by
any standard.

> 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.

I want to look at this more carefully. Its probably worth writing 
a unit test for it.

 
> 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.

I'd actuall prefer to remove those ASSERTS, because they get bypassed
for the production compiles anyway.
 
> 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.

This also needs more investigation. Your patch changes the output
of this function for any given input.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/