Re: [PATCH] StatHist / StatCounters refactoring interim merge

On 14/12/2011 9:19 p.m., Kinkie wrote:
> Hi all,
> attached you can find my proposal for an interim merge of my current
> StatHist / StatCounters refactoring effort.
> What I tried to achieve so far is:
> - c++-refactor StatHist
> - get code out of protos.h, typedefs.h, protos.h and globals.h in
> order to reduce linking dependencies
> - adapt callers
> - document things, or at least try to. Help will probably be needed to
> improve this part.
>
> What was not done yet:
> - Finish c++-refactoring, by using constructors, virtual methods and
> class hierarchy instead of function pointers and explicit
> post-construction initialization
> - change counters to unsigned ints and bigger int sizes (there's an
> open bug on this)
>
> What crept in:
> - some postfix-to-prefix-increment changes. these are harmless, and
> I'll do a specific-purpose sweep on the whole codebase once I'm done
> with this effort.
>
> Code compiles and tests cleanly.
>
> Can you check it out please? Don't be fooled by the size of the patch,
> it's due to the 20 lines of context for each changed line: there are a
> total of about 750 individual changes.
>
> Thanks

Here we go, a quick check...

In Makefile.am;
* you are linking StatHist.cc alongside tests/stub_StatHist.cc. please
don't.
* please ensure that unit tests which are not testing the stats
functionality use the stub_* file,
* that stub_* contents is kept up to date with the class refactoring changes
* preferrably the touched stub_* files get refactored to use the STUB.h
framework as you go.

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

in src/StatCounters.h:
* "AUTHOR: " line at the top of the copyright please so its more visible.
* please adjust teh class documentation comments to doxygen style, even
if you are not changing the texts, something is better than nothing.
* please consider renaming cd_guess_stats to current CamelCase naming
guidelines.
  - 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.

in src/StatHist.cc:
* please use C++ static_cast operator on the xcalloc output instead of
C-casting. several places for this.
* StatHist::clear() - can simply use memset() over the array of int and
faster than a for loop.
* 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)
* 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.
* in the new statHistDeltaMedian() please polish whitespace: s/A,B/A, B/
* please review capacity, bins, min, max fields to see which can be made
private and renamed with suffix '_'.
* in StatHist::enumInit() you should not have to cast to double if you
write constants with decimals. ie "-1.0"
* you seem to be adding whitespace at the end of the file

in src/StatHist.h:
* same with the "AUTHOR:" line
* please comment (before the include) why typedefs.h is a dependency, to
ease removal later.
* s/StatHist.c/StatHist.cc/ to match the new reality. doxygen also has
auto-include macros you can make use of here.
* "/** 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.
* 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.

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

Amos
Received on Thu Dec 15 2011 - 03:07:03 MST