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.
* 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
> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk