Re: [PATCH] StatHist refactoring interim merge, proposed fix for bug 3381

On Tue, Jan 3, 2012 at 3:32 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 3/01/2012 10:11 a.m., Kinkie wrote:
>>
>> Hi all,
>>    here's a new interim merge proposal for the StatHist refactoring
>> effort. This merge aims to close bug 3381 and advance a bit the
>> general StatHist refactoring effort.
>>
>> What it does:
>> - change the type used for StatHist counters to uint64_t (parametric
>> as StatHist::bins_type)
>> - change the index for StatHist bins to unsigned
>> - bring stub_StatHist forward and actually use it, also to remove some
>> objects from tests
>>
>> What crept in:
>> - an initial effort for a StatHist unit test.
>> - some src/Makefile.am cleanup
>>
>>
>
>
> in StatHist.cc:
>  * StatHist::findBin() - float type is quite inaccurate. Can you use double?

Yes. Thanks

>  * StatHist::operator =() - rather than duplicating this code can you make
> it inline?

I'm not sure I understand what you mean.
Please keep in mind that this code is currently in due to the need to
satisfy the squid3 mandatory coding guidelines (StatHist has default
constructor and destructor, so it must have assignment).

Eventually I plan to evolve StatHist to a class hierarchy, with:
StatHistBase: responsible for memory management
StatHistLog, StatHistEnum, StatHistInt: derived from StatHistBase,
each responsible for a different flavor of counting, thus dismissing
the explicit init methods

> in testStatHist:
>  * please define testStatHistEnum() or replace with a TODO.

At the cost of a bit of further creep-in, done. Here's a revised
patch, which on top of the previous one, also:
- actually uses the unit test
- delivers a complete default constructor for StatHist
- adds equality test for StatHist
- adds more stubs to the collection (stub_mem and stub_stmem) and uses them

-- 
    /kinkie