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