Re: CRC Library Formal Review

From: Daryle Walker (darylew_at_[hidden])
Date: 2001-04-17 11:24:41

on 4/15/01 2:02 PM, Jens Maurer at Jens.Maurer_at_[hidden] wrote:

> Here are my review comments for the second round of the CRC library.
> After another round of polishing, it seems ready to go into boost.
> 
> - It may be a good idea to convert the test program to use the
> Boost.Test library, so that the test isn't aborted after the first
> error.

Will do, when I figure out how it works.

> - The library doesn't compile with Comeau C++ in strict mode. See the
> error output at the end of this message.  These are due to applying
> proper two-phase name lookup in this compiler.
> In general, it seems useless that crc_table_t derives from mask_uint_t,
> since the latter contains only static stuff and all references to the
> base class need to be qualified anyway.  Same at crc_basic_t.
> Deriving crc_optimal from crc_table_t is similarly useless.

I got rid of some of the inheritance, and used "using" statements for the
others to carry the names over.

> - When running a como-compiled binary, I got the following error
> 
> [...]
> CRC-32 tests:
> Compute test: ***passed.
> Interrupt test: *****passed.
> Error test: *******passed.
> 
> a.out: crc_test.cpp:409: Assertion `0 == ran_crc_check' failed.
> Augmented-message test: Aborted

I think this is an endian problem.

> - gcc 2.95.3 gives the following error:
> 
> ../../boost/crc.hpp:399: conflicting types for `typename
> boost::detail::mask_uint_t<Bits>::fast
> boost::detail::crc_table_t<Bits,TruncPoly,Reflect>::table_[256]'
> ../../boost/crc.hpp:190: previous declaration as `typename
> boost::detail::mask_uint_t<Bits>::fast
> boost::detail::crc_table_t<Bits,TruncPoly,Reflect>::table_[(1 << 8)]'
> 
> This looks like a bug in gcc.  My efforts to work around that bug
> by defining the table size separately didn't work out.  :-(
> I feel that this needs to be addressed somehow, possibly even by
> redesigning how crc_table_t is defined and/or used.

John Maddock noted the same problem, and gave a solution suggestion.  I
implemented a variant in the latest version (13) in the vault.

> - The source code contains a lot of (forward) declarations of template
> functions, which are not strictly necessary.  It would help my
> understanding of the source code if these wouldn't be there and thus
> clutter up my screen space.   In particular, since the class
> declarations (as opposed to the definitions) don't really convey much
> information about the interface.
> (Having function definitions out-of-line is fine, though.)
> 
> - The source could should be organized differently: The crc_table_t
> is only needed for the crc_optimal implementation, yet it's right at
> the beginning, in front of the crc_basic stuff.   Can't we put that
> in the "optimal CRC" section?

Those forward declarations make a nice summary.  They are also where I put
typedef's and default arguments (function and template).  And it's all in
one place, instead of journeying all over the code to figure out what's
important.  I did do a lot of reorganizing in this update, though.

> - The source line lengths are occasionally >= 80.

I got rid of some of those, but there's probably places I missed.

> - The implementation has done away with ::std::XXX, but it's still
> in the documentation ("Header Synopsis").
> 
> - An "acknowledgement" section may be a good idea, naming the people
> who helped with finding bugs etc. (e.g. Joel Young).
[TRUNCATE Comeau C++ error output]

Will do.

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