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
