Re: [boost] CRC Library Formal Review

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2001-04-15 07:02:50

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.

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

 - 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

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

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

 - The source line lengths are occasionally >= 80.

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

Jens Maurer

Comeau C/C++ 4.2.45.2 (Feb 27 2001 17:23:08) for RedHat_LINUX_INTEL_ELF_BETA
Copyright 1988-2001 Comeau Computing.  All rights reserved.
MODE:strict warnings C++

"../../boost/crc.hpp", line 412: error: identifier "high_bit_fast" is undefined
          fast const           fast_hi_bit = high_bit_fast();
                                             ^

"../../boost/crc.hpp", line 528: error: identifier "sig_bits" is undefined
      return rem_ & sig_bits();
                    ^

"../../boost/crc.hpp", line 560: error: identifier "high_bit" is undefined
      value_type const  high_bit_mask = high_bit();
                                        ^

"../../boost/crc.hpp", line 645: error: identifier "sig_bits" is undefined
       & sig_bits();
         ^

"../../boost/crc.hpp", line 661: error: identifier "init_table" is undefined
      init_table();
      ^

"../../boost/crc.hpp", line 728: error: identifier "sig_bits_fast" is undefined
      return detail::crc_helper<Bits, ReIn>::reflect( rem_ ) & sig_bits_fast();
                                                               ^

"../../boost/crc.hpp", line 777: error: identifier "table_" is undefined
          rem_ ^= table_[ byte_index ];
                  ^

"../../boost/crc.hpp", line 804: error: identifier "sig_bits_fast" is undefined
       ^ get_final_xor_value() ) & sig_bits_fast();
                                   ^

8 errors detected in the compilation of "crc_test.cpp".