Hi Robert, Jeff Garland skrev:
Hi all,
The review of the Robert Kawulak's Constrained Value library begins today December 1, 2008, and will end on December 10th -- I will be the review manager. Please post reviews to the developer list.
I'm basing this review on reading the docs, mostly. 1. I don't like that the exeption object broken_contrait takes an std::string argument since that may throw std::bad_alloc. I'm fine with that in my own code, but I think libraries should try to avoid it. See also http://www.boost.org/community/error_handling.html 2. I prefer functions like change_constraint(c, is_positive()); to be members. We had similar discussion for some function in the Bimap review. Basically, it is nicer for the user to press . to get a list of functions than to search the boost documentation for the right function. This comment applies to all the free-standing functions. 3. In this code: ignoring_even_int j(1); // exception! (cannot ignore construction with invalid value) is there no way to get an assertion instead of the exception? 4. Is your library powerfull enough to implement something like http://biology.nmsu.edu/software/probability/index.html ? If not, what would it take to make that possible? 5. The docs say "Constrained objects can be used as a debugging tool to verify that a program operates only on an assumed subset of possible values. However, it is usually desirable to check the assumptions only in debug mode and avoid the checks in release code for performance reasons" Have you made any benchmarks? In particular, I would be interested in those for bounded_int or bounded_float. (bounded_float/bounded_double,bounded_unsigned etc. are provided, no?) 6. My main use case would be to use constrained floating point values, but the docs state: "Why C++'s floating point types shouldn't be used with bounded objects? [...] there exist a number of myths of what IEEE-compliance really entails from the point of view of program semantics. We shall discuss the following myths, among others: [...] "If x < 1 tests true at one point, then x < 1 stays true later if I never modify x." I don't fully understand this...AFAIK, this is not allowed for an IEEE complient implementation. How could we ever implement *anything* with respect to floats then? Anyway, here's my review vote: *************************'**** I think this library should be accepted into Boost provided that minor changes are done. The docs are nice and clear, and the library gives some quite interesting examples of how powerful the library is. Nice work! I do think that the above issues should be thought really hard about, especially 1,2,3 and 6. If to is not possible, I change my vote to no. best regards -Thorsten