Re: CRC Review

From: Daryle Walker (darylew_at_[hidden])
Date: 2001-03-15 20:54:52

on 3/15/01 3:14 PM, Beman Dawes at bdawes_at_[hidden] wrote:

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

Thanks.

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

Since the two CRC computers are independent, it isn't appropriate to put one
as a "detail."  I've changed crc_fast so its bit size must match that of a
built-in type, so crc_slow is the only way to compute CRCs for arbitrary bit
sizes.

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

I have added a free-function "crc."  Maybe the new names could be something
like "crc_optimal" and "crc_basic".  I don't want the names too long; the
headers I write already tend to be verbose.

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

OK.

> *  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 have extensively changed the interfaces.  Regular member functions are
used to read data and return the checksum.  The fast computer still keeps
its "operator()( unsigned char )" to be used with std::for_each.  The
"operator *()" was replaced with an "operator ()()" to let the object act as
a generator.  Both these operators call regularly-named member functions.
There's also some new stuff.

> *  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.
[TRUNCATE]

I think the CRC timing code Vladimir Prus posted has a conventionally
programmed routine.  I don't know the *.zip format, so that could be hard if
its CRC data and checksum are deeply embedded; I don't want to have to write
a *.zip parser.

-- 
Daryle Walker
Mac, Internet, and Video Game Junkie
darylew AT mac DOT com