Re: [PATCH] StatHist / StatCounters refactoring interim merge

On 16/12/2011 7:05 p.m., Kinkie wrote:
>> It still does make sense to check the capacity bounds. Consider A
>> initialized to 10 entries, and B initialized to 100 entries copying B into
>> A. As I read it now the copy only checks that both are initialized, and will
>> allow A at position 11+ to be referenced during the copy. We avoid it so far
>> by having fixed histograms in the caller code, but it is not actually a
>> guarantee anywhere by the StatHist API.
> Agreed.
>
>> What I'm looking for is some surety that the A will either fail or grow to
>> meet the B size requirements. Given that the callers are using Hist
>> consistenly regarding size I think it can probably be an assert for now
>> instead of adding growth complexity.
> v2 of the patch grows. It adds 3 lines of code or so, and it was in
> the todo list anyways.
>
>>>> * StatHist::findBin()- appears to be able to return -1 array index if
>>>> capacity is zero. Please make it return unsigned, and return 0 on the
>>>> line
>>>> doing (bin = 0) presently.
>>> Ok for not returning -1, but may I delay turning signed to unsigned
>>> for second round?
>>
>> Fine by me.
>>
>>
>>>> * please review capacity, bins, min, max fields to see which can be made
>>>> private and renamed with suffix '_'.
>>> I intentionally left them protected as I'd like to implement a class
>>> hierarchy.
>>> Why the trailing underscore? There is no name conflicts.
>>
>> Fine. Just keep in mind that on the copy constructor when you get around to
>> adding "max(...), min(...)" it turns into the libc max/min functions or
>> template instantiations.
> you convinced me on this one. Renamed.
>
> Any objections to merging?

None from me now.

Amos
Received on Fri Dec 16 2011 - 07:03:26 MST