> 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? Thanks -- /kinkie
