On 01/04/2012 08:28 AM, 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().
Do we use StatHist::operator "=="? If not, I suggest not adding it
because the added code may return false for histograms that are
essentially the same for many practical purposes. In general, what
histograms are considered the same depends on the caller use of
statistics so if we do not have any callers, let's not add this code.
No need to repost the patch with this minor change, of course.
Thank you,
Alex.
Received on Mon Jan 09 2012 - 19:18:58 MST
This archive was generated by hypermail 2.2.0 : Tue Jan 10 2012 - 12:00:04 MST