CRC Review

From: Beman Dawes (bdawes_at_[hidden])
Date: 2001-03-15 15:14:02

[These are my personal opinions, not "review manager" comments.]

I think the CRC Library should be accepted by Boost, subject to the 
portability and testing questions raised by others being resolved.

Issues:

*  Names crc_slow and crc_fast.  I see the benefit of retaining crc_slow, 
but it seems to me that its uses are in understanding CRC's and in 
testing.  Thus I think crc_slow should be moved to the boost::detail 
namespace, the code moved to a separate header in boost/detail, and the 
documentation adjusted so that the casual user never sees crc_slow.

I'd also rename crc_fast to just plain crc.  If a free-function gets added 
(as suggested by another reviewer), and that gets named crc, then let's try 
to figure out some name other than crc_fast for the class.  "_fast" won't 
look like a very good name if some comes up with a faster 16-bit table 
implementation.  Less importantly (if it is in namespace detail) I'd 
probably change "crc_slow" to "basic_crc".

*  Typedefs.  I'd like to second John Maddock's suggestion for including at 
least a few typedefs like crc_ccitt, crc16, crc32.  I didn't find the 
counter argument convincing.  If they aren't "standard" in an ISO sense, it 
doesn't matter.  Just document them as "useful" and point out, for example, 
that crc32 is used by "Ethernet, ZIP files, and some other computer 
applications."  Even just three typedefs will help users get started, and 
in fact is all most will ever need.

*  Interface.  I'll just quote John Maddock's comment, which I agree 
completely with:

 > I don't like the use of operators in the crc_fast interface - with the
 > possible exception of operator()(unsigned char) I don't see what these
 > gain you: crc's are not iterators, smart pointers, or even function
 > objects (strictly speaking function objects aren't supposed to have
 > state - but this is an open issue - hence the exception for
 > operator()(unsigned char) for use with std::for_each).

*  I'm nervous about the limited test coverage, particularly since a number 
of postings point to compiler or platform issues.  I'd like to see the test 
coverage expanded.  For example, using the Boost.Random library to generate 
a long sequence of input characters, and then check crc_fast against (1) 
crc_slow, and (2) a completely independently implemented function if you 
can lay your hands on one.  Another possibility it to write a little 
program to check crc's on .zip files.  That is a nice test as it is 
completely independent. It would be reassuring and I'm sure you would get 
volunteers to test on various compiler/platform combinations.

Anyhow, thanks to Daryle for working on CRC's.  Having an easy-to-use 
library may encourage programmers to use CRC's more, resulting in an 
increase in data reliability.

--Beman