On 16/12/2011 12:18 p.m., Kinkie wrote:
>> Here we go, a quick check...
>> * StatHist::clear() - can simply use memset() over the array of int and
>> faster than a for loop.
> It is actually only called when explicitly deinitializing the StatHist
> objects; it's a candidate for removal; IMVHO this is premature
> optimization.
Fine then.
>> * StatHist::operator = - please enact that assumption removal and add more
>> bounds checks (ie if init has been called the Dest capacity must large
>> enough to copy into)
> I was hoping to do this later on in the refactoring, but I'll just
> bite the bullet. The function this operator= is the porting of asserts
> on conditions that do not make sense in a c++ world with the
> associated invariants. e.g. my plan is never to allow the histogram's
> storage be NULL for the lifetime of the histogram object itself, and
> it doesn't make sense to enforce equal capacity requirements when
> we're going to copy contents over; it's much more consistent with
> expected behaviour to just resize and copy everything. The asserts
> don't trigger on current code, which means that this operator is only
> used where it makes sense, but why add artificial limits when it's not
> needed?
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.
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.
>
>> * 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.
Amos
Received on Thu Dec 15 2011 - 23:47:45 MST
This archive was generated by hypermail 2.2.0 : Fri Dec 16 2011 - 12:00:10 MST