From: Kevin Lynch (krlynch_at_[hidden])
Date: 2003-12-19 17:28:32
Thorsten Ottosen wrote:
> Dear all,
>
> The review of Fernando Cacciola Numeric Conversion library starts today (8th
> of Dec.) and runs until the
> 22th of December.
>
I regret to report that I have not been able to make the time for a
complete and proper evaluation of the nitty gritty details of the
current design and implementation, and I won't be able to before the end
of the the review period, but I was deeply involved with the discussions
about a year ago that let Fernando to build this library. Hopefully
makes up for the shortcomings in my evaluation this time around :-)
Apologies if I bring up something that has already been discussed and
addressed.... pretend I didn't mention it here.
> a.. What is your evaluation of the design?
>
I like the overall design. I think that the design is well seperated
into orthogonal components, is complete, and doesn't attempt to promise
any more than it delivers.
> b.. What is your evaluation of the implementation?
I regret that I have not evaluated the current implementation beyond a
quick looksee.
>
> c.. What is your evaluation of the documentation?
>
As noted by others, the documentation could use some help ... but what
programmer's documentation doesn't need some help? :-)
Specifically, as others have mentioned, there is a need for more
examples to help people implementing their own traits and converters.
Additionally, I have the following comments and/or questions:
1) The Overview really needs to have a few motivating use cases, right
up front, on the problem the library attempts to solve. This is
different from the need for examples of how to implement the traits
needed to work with UDTs
2) In the definitions section:
a) Nitpick: I think that your definition of "floating numeric types" is
too narrow; it leaves a gap in the numeric types that isn't filled by
the other types. By defining floating types to be "[where] the abstract
values represent real numbers", you are only admitting numeric types
such as float/double etc with vanishing density. You leave out the
class of numeric types with fraction parts and finite density (for
example, a type that covers monetary values with a finite number of
decimal digits are not included in your classification scheme). I think
you intended to include those kind of types in floats, because in at
least two places, you mention that "floating types have a density << 1".
I think you need to change the definition.
b) Another nitpick: You define next() and prev() in the section on range
and precision, and use that to define a numeric set by iteration. Fine.
In the section on rounding, you use next() and prev() to define a
"correct rounding" of an abstract value V to its rep v = rep(V) to be
those roundings where v == prev(V) or v == next(V). This is incorrect
when the abstract value can be exactly represented: if V = 1 and T =
int, then next(V) = 2, not 1. I think you either need to redefine
correct rounding in terms by adding the exactly representable case
explicitly, or requiring "v == prev(next(V)) or v == next(prev(V))"
which solves the problem. The former is clearer, the latter is more
succinct and prettier to my eyes :-)
c) In the section on Standard Conversions, you use "sequel" a few times.
I can't figure out what you intended to say. Did you mean "sequence"
perhaps?
d) Comment: It seems strange that the standard demands that integers
always be convertable to floats ... with this requirement, it is not
possible for an implementation with the minimal compliant floating point
(FLOAT_MAX = 1E+37) and big integers (128 bit longs, for instance, with
ULONG_MAX approx 3.4E+38) to be compliant. Granted, that would be a
pretty strange platform, but it seems a bit wacky to preclude those
implementations from conformance. Anyone know of a good reason why the
C++ Standard demands that integer to floating point _always_ be defined,
while C99 explicitly does not (C99 6.3.1.4/2 has nearly identical
language to C98 4.9, but additionally has the line: "If the value being
converted is outside the range of values that can be represented, the
behavior is undefined.")???? I don't have a copy of C89, so I don't
know if that is a change to C or an incompatibility added by C++. It
isn't a "Defect" in the standard per se, but it is a little strange.
3) Header ... converter_policies.hpp
You give explicit values in the documentation to the values of enum
range_check_result, which I don't think you should do in the
documentation ...
> d.. What is your evaluation of the potential usefulness of the library?
This library should not only be extremely useful, it plugs a yawning gap
in the C++ standard library. It should be useful in many numerically
oriented libraries and applications where the screwy C++ conversion
rules can really bite you.
>
> e.. Did you try to use the library? With what compiler? Did you have any
> problems?
>
I have not tried to compile or use the current implementation, but I did
use the older implementation which Fernando made available a while ago;
I had compiler related issues (Redhat gcc 2.96) at the time to work
around that probably aren't relevant anymore.
> f.. How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
This time around, I was only able to put in about two hours into
studying the documentation and examples, so probably more than a quick
read, but less than an in-depth study.
>
> g.. Are you knowledgeable about the problem domain?
Moderately ... I use numerics regularly in my job, but I have no formal
training in the field. I get bitten by the problems this library solves
regularly enough to know the need.
>
> h.. Do you think the library should be accepted as a Boost library?
I vote unconditionally for acceptance.
-- ------------------------------------------------------------------------------- Kevin Lynch voice: (617) 353-6025 Physics Department Fax: (617) 353-9393 Boston University office: PRB-361 590 Commonwealth Ave. e-mail: krlynch_at_[hidden] Boston, MA 02215 USA http://budoe.bu.edu/~krlynch -------------------------------------------------------------------------------