[boost] CRC review

From: John Maddock (John_Maddock_at_[hidden])
Date: 2001-03-11 07:31:52

Following on from yesterday, I've been investigating the CRC problems some
more:

I had to modify the code (again) to get it to build with gcc, the good news
is that it runs successfully under gcc (updated code attached -
fast_crc::operator* contained some borderline illegal code).

With VC6 I've tracked the problem down to the reflect code - for some
reason reflect is getting called not as reflect<8>, but as reflect<1>, this
is the cause of the problem here, and I've added some debugging assertion
code to help track this down (see attached header). However what I don't
have is any kind of solution.

I still don't know what the problem is with the Borland compiler - except
that it's nothing to do with reflect which is working fine here.

At this point some more general comments are probably in order:

Implementation:
~~~~~~~~~~
What on earth is: 
~( ~(value_type( 0u )) << Bits
supposed to mean, isn't ~(~0u) alway zero or am I missing something?  If so
some kind of explanatory comment would be nice :-)

Names:
~~~~~

Don't care what you call crc_slow - it's probably only of academic interest
anyway?  But crc_fast may as well just be called "crc" IMO.

Also most people will never use the crc template directly, instead I would
like to see a set of typedefs for the "standard" crc parameters.  Most
people (like me!) will have no idea which parameters they should be using
in any case and will just want a particular crc implementation:

typedef crc_fast<2, 0x1021, 0xFFFF, 0, false, false>  crc_ccitt;
typedef crc_fast<2, 0x8005, 0, 0, true, true>  crc16;
typedef crc_fast<4, 0x04C11DB7, 0xFFFFFFFF, 0xFFFFFFFF, true, true>  crc32;

The docs should then reflect the fact that most people will be using these
rather than the templates directly - indeed it should be emphasised that
they should not use the templates directly unless they really *really* know
what they're doing!

Interface:
~~~~~~

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 would change operator* to:

    value_type  value() const;

and operator() to :

    void  add( unsigned char byte );
    template <class InputIterator>
    void  add( InputIterator bytes_begin, InputIterator bytes_end );
    void  add( void const *buffer, ::std::size_t length );

I would also add overloads for char and signed char for completeness, the
choice of the name "add" is moot here, maybe someone can come up with
something better?

Again that's all for now...

- John Maddock
http://ourworld.compuserve.com/homepages/john_maddock/