Re: [PATCH] StatHist / StatCounters refactoring interim merge

On Fri, Dec 16, 2011 at 6:02 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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.

Doh! Added. *sigh*.

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

Ok.

>> +    StatHist(const StatHist&);
>
> Sorry if I missed its definition, but if you do not want to define it,
> please make it private.

Making it private. It's really not needed.

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

We will, in the next merge step. But for now it's not needed. De-virtualizing.

>> +    /** 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.

These histograms are instantiated at initialization time and destroyed
at cleanup time.
The clear() method is the remnants of a C explicit destructor function.
I chose for this intermediate step to split the destruction work in
two: clear() clears the contents, destructor deallocates.
Eventually clear will just go, as it's not really used anywhere except
in what remains of the explicit deinitialization code.

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

It can be done, but I'd prefer not to: why risk adding bugs to code
which is called only once (at exit time) and is dying anyways?

>> +    StatHist() : scale(1.0) {};
>
> Extra semicolon.

Ok.

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

Yes. I'm doing that in step two of the refactoring.

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

I'll just remove it. I don't mean to imply any of this.

> Thank you,

Thanks!

-- 
    /kinkie