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

 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.

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

 Will review later.

 Amos
Received on Tue Jan 03 2012 - 21:57:13 MST