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

On 5/01/2012 4:28 a.m., Kinkie wrote:
> On Wed, Jan 4, 2012 at 7:05 AM, Amos Jeffries<squid3_at_treenet.co.nz> wrote:
>> On 4/01/2012 11:42 a.m., Kinkie wrote:
>>>>> 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.
>>>
>> in Makefile.am:
>> * tests_testStatHist_SOURCES - alphabetical please. The linker does not care
>> about order in *_SOURCES and sorted its far easier to find duplicate
>> linkages.
>> * tests_testStatHist_LDADD - $(COMPAT_LIB) must be last on the list after
>> all the convenience libraries, (ie libmiscutil.la). Only system libraries
>> may go after it.
>>
>> in StatHist.h
>> * operator=() - xfree() can take a NULL ptr safely.
>> * destructor body can be replaced with safe_free(bins); or probably even
>> just xfree(bins); now that there is a proper constructor.
>>
>> in stub_mem.cc
>> * compat/xalloc.h does not need including directly.
>> * please include protos.h instead of typedefs.h, with the comment "mem*()
>> definitions still in protos.h" (on a separate line to teh #include).
>> - when the protos are moved whatever header they move to will have to take
>> care of locating FREE as well, just like protos.h does now.
> Hi,
> v4 of the patch attached. It covers all of the above, plus it
> changes an assert to a condition in to cover a shutdown race in
> StatHist::count().

K. I dont think StatHost destructor needs to be quite that thorough
about clearing the basic type variables, but doesnt matter either way.

+1.

Amos
Received on Thu Jan 05 2012 - 03:54:44 MST