Review: CRC Library

From: Doug Gregor (gregod_at_[hidden])
Date: 2001-03-12 00:37:34

Disclaimer: I know very little about CRC computations, so I can make no 
comment as to the algorithmic correctness of the CRC library.

Comments
----------	
* Naming: 
        - If crc_fast will be used most (all?) of the time, perhaps it should be 
renamed just to "crc"?
        - operator* seems an awkward choice for returning the current CRC value. 
Perhaps a member function "value" would be more readable.

* Interface:
        -crc_slow takes a the number of bits whereas crc_fast takes a number of 
bytes, which may be confusing. Suggestion: use bits for both and add a static 
assert ensuring that bits is a multiple of 
std::numeric_limits<unsignedc har>::digits.

        - The ability to specify an initial remainder as part of the constructor 
might be useful. For instance, if you wanted to pick up calculation of a CRC 
but only have the remainder still (not the crc_fast object used).

        - Free functions that calculate CRCs for a block of data and return the 
resulting CRC value would easy usage.

        - The addition of typedefs for common CRC types would greatly help.

* Implementation:	
        - The nontype template parameters TruncPoly, InitRem, and FinalXor all have 
type "unsigned long". The parameters' types would more accurately be 
described by 
"typename detail::bit_traits<(Bytes * detail::bits_per_byte)>::fast_type".

        - The following two expressions, ~( ~(value_type( 0u )) << Bits ) and 
~( ~(fast_type( 0u )) << Bits ), cause errors on Comeau 4.2.44 because 
shifting by >= the number of bits in an integral type invokes undefined 
behavior. Suggestion:
template<typename Type> struct all_bits_significant { 
  BOOST_STATIC_CONSTANT(Type, value = ~((Type)0));
};

template<typename Type, int Bits> struct low_bits_significant {
  BOOST_STATIC_CONSTANT(Type, value = ~( ~(value_type( 0u )) << Bits ));
};

Then use a meta-IF, i.e., 
IF<(Bits ==std::numeric_traits<fast_type>::digits),
    all_bits_significant<fast_type>,
    low_bits_significant<fast_type, Bits> >::type::value

        - The two aforementioned expressions really need a comment

* General:
        - The test code is very short. Is a more comprehensive testsuite available 
or are these tests sufficient?

I think the CRC library should be accepted into Boost provided the 
portability concerns (i.e., that John Maddock has reported) are addressed. 

        Doug