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
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/crc.hpp:190: previous declaration as `typename
> 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]
-- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT mac DOT com
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk