Re: [PATCH] StatHist / StatCounters refactoring interim merge

On 12/15/2011 04:18 PM, Kinkie wrote:

> Attached second version of the patch.

> +StatHist&
> +StatHist::operator =(const StatHist & src)

The above operator is missing assignment-to-self protection.

HttpHeaderStat is also missing explicit copying methods, but at least it
does not implement anything new that is broken so I cannot insist you
add those missing methods. Here, since we do add an operator, we should
implement it correctly.

> + StatHist(const StatHist&);

Sorry if I missed its definition, but if you do not want to define it,
please make it private.

> virtual ~StatHist();

Do we ever inherit from StatHist()? If yes, clear() and piossibly other
general methods should be virtual as well. If not, the destructor should
not be virtual.

> + /** clear the contents of the histograms
> + *
> + * \todo remove: this function has been replaced in its purpose
> + * by the destructor
> + */
> + void clear();

Not sure what you mean by "replaced in its purpose", but clear()ing a
histogram and destructing it are two different actions. It is possible
to replace all clear() calls with code to destruct and then create the
histograms, but it feels wrong to do that from both simplicity and
performance point of view.

I would also echo Amos' comments that clear() should use memset() for
the bins.

> + StatHist() : scale(1.0) {};

Extra semicolon.

> static void
> httpHeaderStatInit(HttpHeaderStat * hs, const char *label)
> {
> assert(hs);
> assert(label);
> memset(hs, 0, sizeof(HttpHeaderStat));
> hs->label = label;
> - statHistEnumInit(&hs->hdrUCountDistr, 32); /* not a real enum */
> - statHistEnumInit(&hs->fieldTypeDistr, HDR_ENUM_END);
> - statHistEnumInit(&hs->ccTypeDistr, CC_ENUM_END);
> - statHistEnumInit(&hs->scTypeDistr, SC_ENUM_END);
> + hs->hdrUCountDistr.enumInit(32); /* not a real enum */
> + hs->fieldTypeDistr.enumInit(HDR_ENUM_END);
> + hs->ccTypeDistr.enumInit(CC_ENUM_END);
> + hs->scTypeDistr.enumInit(SC_ENUM_END);
> }

Can this be moved to HttpHeaderStat constructor?

> + * AUTHOR: Francesco Chemolli

I cannot object to this, but it seems a little unfair towards the folks
who wrote the code you have refactored. Suddenly, they became less of an
author. Again, no objection or offense from me personally.

Thank you,

Alex.
Received on Fri Dec 16 2011 - 17:02:29 MST