Boost logo

Boost :

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2001-03-25 12:20:46


I've had a look at crc version 9 from the boost files section.

Here are a few comments:

boost/crc.hpp

 - The exact_uint_t<> template is once declared and only later defined.
That makes for a lot of repeated #ifdef and, as an implementation detail,
doesn't buy much clarity either. Could we omit the declarations?

 - CRC is translated as "cyclic redundancy code". I always remembered
it as "cyclic redundancy check". Are there any authoritative sources
on this?

 - Using ::std::XXX (note the leading ::) doesn't buy anything
(we will never have a "std" sub-namespace within the boost namespace),
but makes it possible to get <: digraph surprises when used in
template parameters. Besides, it's two more redundant characters
to read.

 - The "const" members in a crc_basic<> mean that the compiler cannot
generate a default assignment operator. Is there any reason why
a crc_basic<> object cannot behave like a value (i.e. Copyable and
Assignable)?

 - In class reflector, "reflection &= ~mask" seems superfluous because
you start off with "reflection = 0" anyway.

 - "bit-swaps are appearent" sounds like broken English to me.
What about "noticeable" or "detectable"?

 - Is there any reason why bitset_reflector is a class instead of
a simple global function? It doesn't have any state, obviously.
Same with reflector.

 - crc_optimal<>::process_byte calls ...::process_bytes which in turn
calls ...::process_block. And sizeof(unsigned char) is guaranteed
to be 1, btw. Wouldn't it be better to call process_block directly?

 - crc_tabulator could be a simple function as well, since the "did_init"
logic is already implicit in its use for initializing a static block-scope
variable in crc_optimal<>::process_block. That reduces at least two
levels of indentation as well :-) Plus, I'd use a static class member
to hold the table, because this saves checking whether we need to
init the table at every call to process_block.

 - Just for the record: I don't like your style of function
declarations. It spreads out on too many lines, even for trivial
stuff such as inline "get" member functions.

Documentation
 - "...built-in unsigned integral type that contains at least Bits bits."
Can we say "represents" instead of "contains"? Some strange hardware
may actually have ints comprising of 36 bits, but only use 32 of them to
make an int. (Is that legal under the current language rules?)

 - The "Rationale" needs an additional introductory sentence before
it's able to make sense. something about "communications".

 - Truncated polynomial: "same register type": C++ does not have a
notion of "register" (except in some arcane C compatibility corners),
so you should either explain its use here or say "data type".

 - Final XOR value: "combined with a set value": I'm easily confusing
this with "set" as in "sets of numbers". Can we say "defined" or "some"?
"via" is in italics for no apparent reason.

 - Reflected input: "etc" is in italics for no apparent reason.

 - Reflected (remainder) output: "before" is in italics for no apparent
reason.

 - In crc_basic, you're using a bitset<> to store the interim remainders,
but the value type is limited to an integral type. This seems inflexible
at least.

 - Theoretical CRC Computer, last sentence: "It waste[s] space".

 - Optimized CRC Computer, last sentence: "this guideline is broken"
Would "this rule is not adhered to" be better?

 - CRC Function: "The CRC parameters are passed through the template
parameters like the optimized CRC computer. The function uses a matching
version of the optimized CRC computer for its internals." sounds really
weird to me. What about "The CRC template parameters are passed through to
the template parameters of the optimized CRC computer (see above). The
thusly constructed object is used to implemented the function."

 - CRC Examples: There is a crc_16_type and an identical crc_arc_type,
while there is only a single crc_32_type useful for lots of purposes.
Please explain the other crc_XXX_types as well, and if possible,
reduce the redundancy of typedef's.

 - Usage: "The two class template[s]"

 - Usage: Replace "slow" by "theoretical"

 - Usage: "instantiating objects" --> instantiated objects

Jens Maurer


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk