Re: Comments on crc9

From: Daryle Walker (darylew_at_[hidden])
Date: 2001-03-26 23:28:58

on 3/25/01 7:20 PM, Jens Maurer at Jens.Maurer_at_[hidden] wrote:

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

Is it OK to define template specializations without declaring them,
especially if you only have a forward declaration for the general version?

I'm wondering if these could become part of <boost/integer.hpp>?

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

I don't think there's any authority on this.  I think I had it as "check,"
but changed it because my main reference material used "code."

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

I space out the template arguments because I got hit by the "<:" digraph
situation when I didn't.  I think I started the "::std" from seeing a
diagram someone generated from running Doxygen on a Boost file.  The diagram
thought unadorned "std" items belonged in a "boost::std" namespace.  I know
we don't (currently?) use Doxygen, but I wanted to pre-empt any problems
with future diagrams.  I don't know if its worth it.

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

No major reason.  I just wanted to protect myself from accidentally changing
a parameter value.

[SNIP comments about reflection coding]
> - 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.

I think I changed it because I heard that compilers inline function objects
better than functions.

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

I guess.  I've heard that pointer arithmetic is proper only if the original
pointer and the pointer value after adding are both part of the same array
(or to one unit past the array).  Is this true?  If so, how should I handle
single bytes, copy it into an one-element array?

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

I had it as a simple function; I changed it for the same reason as the
reflectors.  Wouldn't the class-static member be contradictory to the simple
function idea?  The table cannot be a class-static member of crc_optimal,
since several versions of crc_optimal will share a table.  (The optimal CRC
computer has six template parameters, the tabulator only uses three of
them.)

I originally had the table be a class-static member of a helper class, but I
couldn't define it correctly.  I made the class declaration, and thought I
defined the class-static members correctly outside the declaration, but the
linker thought the member didn't exist.  Can you show us an example of how
to declare _and_ define a class-static array member of a class template?

[SNIP documentation wording nits]
> - 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.

I thought that these Latin(?)-derived words get italicized.  Does anyone
have a style manual on this issue?

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

It's to emphasize the order of the two output parameters, reflected
remainder and final-XOR value.

> - 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.
[TRUNCATE more wording nits, request to explain CRC examples]

What do you mean by this?

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