Hi Chris, Thanks for looking at this.
There is some rudimentary testing using streams in CollectAndSummaryStatisticsTest.java. I think we should enforce constraints where we reliably can: 1) count >= 0 2) count = 0, then min/max/sum are ignored and it’s as if the default constructor was called. (I thought it might be appropriate to reject since a caller might be passing in incorrect information in error, but the defaults for min/max are important and would be a burden for the caller to pass those values.) In this respect having count as the first parameter of the constructor would be useful. 3) min <= max Since for count > 0 the constraints, count * max >= sum, count * min <= sum, cannot be reliably enforced due to overflow i am inclined to not bother and just document. Note this is gonna be blocked from pushing until the new Compatibility and Specification Review (CSR) process is opened up, which i understand is “soon"ish :-) but that should not block adding some further tests in the interim and tidying up the javadoc. Paul. > On 6 Apr 2017, at 07:08, Chris Dennis <chris.w.den...@gmail.com> wrote: > > Hi Paul (et al) > > Like all things API there are wrinkles here when it comes to implementing. > > This patch isn’t final, there appears to be no existing test coverage for > these classes beyond testing the compensating summation used in the double > implementation, and I left off adding any until it was decided how much > parameter validation we want (since that’s the only testing that can really > occur here). > > The wrinkles relate to the issues around instances that have suffered > overflow in count and/or sum which the existing implementation doesn’t defend > against: > > * I chose to ignore all parameters if 'count == 0’ and just leave the > instance empty. I held off from validating min, max and count however. This > obviously 'breaks things’ (beyond how broken they would already be) if count > == 0 only due to overflow. > * I chose to fail if count is negative. > * I chose to enforce that max and min are logically consistent with count and > sum, this can break the moment either one of the overflows as well (I’m also > pretty sure that the FPU logic is going to suffer here too - there might be > some magic I can do with a pessimistic bit of rounding on the FPU > multiplication result). > > I intentionally went with the most aggressive parameter validation here to > establish one end of what could be implemented. There are arguments for this > and also arguments for the other extreme (no validation at all). Personally > I’m in favor of the current version where the creation will fail if the > inputs are only possible through overflow (modulo fixing the FPU precision > issues above) even though it’s at odds with approach of the existing > implementation. > > Would appreciate thoughts/feedback. Thanks. > > Chris > > > P.S. I presume tests would be required for the parameter checking (once it is > finalized)? > > <8178117.patch>
