Re: [PATCH] StatHist / StatCounters refactoring interim merge

> Here we go, a quick check...
>
> In Makefile.am;
> * you are linking StatHist.cc alongside tests/stub_StatHist.cc. please
> don't.

Fixed.

> * please ensure that unit tests which are not testing the stats
> functionality use the stub_* file,

Ok.

> * that stub_* contents is kept up to date with the class refactoring changes

Done.

> * preferrably the touched stub_* files get refactored to use the STUB.h
> framework as you go.

Done.

> in src/PeerDigest.h:
> * please place the comments about includes on a separate line (case in
> point: "for cd_guess_stats")

Done.

> in src/StatCounters.h:
> * "AUTHOR: " line at the top of the copyright please so its more visible.

Done.

> * please adjust teh class documentation comments to doxygen style, even if
> you are not changing the texts, something is better than nothing.

Ok.

> * please consider renaming cd_guess_stats to current CamelCase naming
> guidelines.

Done.

>  - ideally the fields would change to current namign scheme as well,
> especially since you are already touching almost every mention of them for
> the function refactoring.

Doing it for StatHist fields, not for the others for now - there's
simply too many of them

> in src/StatHist.cc:
> * please use C++ static_cast operator on the xcalloc output instead of
> C-casting. several places for this.

Done. two places.

> * StatHist::clear() - can simply use memset() over the array of int and
> faster than a for loop.

It is actually only called when explicitly deinitializing the StatHist
objects; it's a candidate for removal; IMVHO this is premature
optimization.

> * StatHist::operator = - please enact that assumption removal and add more
> bounds checks (ie if init has been called the Dest capacity must large
> enough to copy into)

I was hoping to do this later on in the refactoring, but I'll just
bite the bullet. The function this operator= is the porting of asserts
on conditions that do not make sense in a c++ world with the
associated invariants. e.g. my plan is never to allow the histogram's
storage be NULL for the lifetime of the histogram object itself, and
it doesn't make sense to enforce equal capacity requirements when
we're going to copy contents over; it's much more consistent with
expected behaviour to just resize and copy everything. The asserts
don't trigger on current code, which means that this operator is only
used where it makes sense, but why add artificial limits when it's not
needed?

> * StatHist::findBin()- appears to be able to return -1 array index if
> capacity is zero. Please make it return unsigned, and return 0 on the line
> doing (bin = 0) presently.

Ok for not returning -1, but may I delay turning signed to unsigned
for second round?

> * in the new statHistDeltaMedian() please polish whitespace: s/A,B/A, B/
Done.
> * please review capacity, bins, min, max fields to see which can be made
> private and renamed with suffix '_'.

I intentionally left them protected as I'd like to implement a class hierarchy.
Why the trailing underscore? There is no name conflicts.

> * in StatHist::enumInit()  you should not have to cast to double if you
> write constants with decimals. ie "-1.0"

Done.

> * you seem to be adding whitespace at the end of the file

Removed.

> in src/StatHist.h:
> * same with the "AUTHOR:" line

Done

> * please comment (before the include) why typedefs.h is a dependency, to
> ease removal later.

It turns out it's not even needed, by moving a couple of typedefs out of it.
Removed.

> * s/StatHist.c/StatHist.cc/ to match the new reality. doxygen also has
> auto-include macros you can make use of here.

I've just moved the notes to the init function

> * "/** Default constructor" no need to state the obvious. Also, why does
> this exist if its not enough to create an object properly? if it is
> transitory please state that to avoid abuses meanwhile.

Ok. Yes, it's transitory. *init has to go.

> * re statHistEnumDumper and statHistIntDumper globally defined;  you could
> start the class hierarchy by creating StatEnumHist and StatIntHist childs
> with specialized init and dump routines to simplify.

IntHist is not even needed. My plan is to define StatEnumHist and StatLogHist.

> Also:
> * please check whether stub_StatHist.cc and StatHist.cc actually need
> squid.h

They don't. Removed.
Attached second version of the patch.

Thanks!

-- 
    /kinkie