Boost logo

Boost :

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.

* 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),
    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.


Boost list run by bdawes at, gregod at, cpdaniel at, john at