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

On Tue, Jan 3, 2012 at 10:57 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On Tue, 3 Jan 2012 17:52:34 +0100, Kinkie wrote:
>>
>> 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.
>
>
> You have duplicate code in StatHist.cc and stub_StatHist.cc with notes about
> keeping them in sync. that is prime candidate for inlining that particular
> duplicated function. The whole point of having stub_* is to prevent an
> active version of the stubbed object being linked. If the stub code gets run
> the test is broken.
>  So this method function should be inline if it needs defining when the stub
> object definitions are linked.
>
>
>
>> 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).
>>
>
> I know. That is irrelevant to this particular code duplication issue.

Now I get it. v3, attached, addresses this.

>> 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
>
>
> Will review later.

Thanks

-- 
    /kinkie