[boost] Nuemric Conversion review

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2003-12-18 08:02:20

Hi Fernando,

here's my review of your library.

  a.. What is your evaluation of the design?
-------------------------
It seems ok, but I lack specific details about how the traits are provided,
eg, as free-standing or not.
I can't see if this will affect the desing of policy classes etc.

  b.. What is your evaluation of the implementation?
-------------------------
seems very tidy.

  c.. What is your evaluation of the documentation?
-------------------------
In some areas plentyful, but lacking more examples and getting started
tutorial for the advanced stuff.

  d.. What is your evaluation of the potential usefulness of the library?
-------------------------
numeric cast is already useful, and if this version is better, then that is
good. The bounds class also
repairs some stuff in the standard.

  e.. Did you try to use the library?  With what compiler?  Did you have any
problems?
--------------------------
 see below.

  f.. How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?
--------------------------
4-5 hours trying to understand it all. This is definitely not an in-depth
study.

  g.. Are you knowledgeable about the problem domain?
--------------------------
intermediate

  h.. Do you think the library should be accepted as a Boost library?  Be
sure to say this explicitly so that your other comments don't obscure your
overall opinion.
--------------------------
My answer is "yes" given the documentation ca be properly updated. I also
need assurance about the design of the traits and how it will affect policy
classes.

Below is some more comments.

1. test
2. documentation

-----------------------------
tests
-----------------------------
I tried compiling the test with

(a) vc7.1:
   only bounds_test.cpp could compile, test fine btw
(b) Comeau 4.3.3:
   everything works except udt_support_test. 1 error:

Testing UDT conversion with default policies
test_helpers3.cpp(74): error in "call_test_main":
converter<MyUDT::UDT<double>,M
yUDT::UDT<int>>::convert(6155)= 6155. Expected:6155.66
Testing UDT conversions with custom range checker

*** 1 failure detected in test case "call_test_main"
(c) g++ 3.3.1 (cygwin) perfect test

-----------------------------
documentation
-----------------------------
There have already been said much about improvements of the docs, so I won't
go into
too muh detail.

I do have a question that I asked you about earlier before the review. What
is the difference between
an abstract number and a real number. From maths we are pretty used to the
notion of a real number, so if they
are the same, I think you should just use that term.

from section C++ Arithmetic Types:
" Use unsigned types only when you need modulo arithmetic or very very large
numbers. Don't use unsigned types just because you intend to deal with
positive values only (you can do this with signed types as well)."

I agree on this, but I don't understand the "very very large" part. If you
want very big numbers, you would probably need more than a built-in type.

The section "Exact, Correctly Rounded and Out-Of-Range Representations"
mentions 1 ulp, but that has not been defined anywhere.

Remember to define 'iff' for those of us that have learned 'iff'.

In general I will say that I find the documentation good. Some might be put
off by the math, but it is certainly good for a library like this.

Numeric Cast.

what is improved in the new version?

Why is the return  type not just Target?
template<typename Target, typename Source> inline
typename boost::numeric::converter<Traget,Source>::result_type
numeric_cast ( Source arg )
{
   return boost::numeric::converter<Traget,Source>::convert(arg);
}there is no docs for  "(see bad_numeric_cast, positive_overflow and
negative_overflow)."The documentation specifies exceptions like this:
virtual const char *what() const // throw()
        {  return "bad numeric conversion: overflow"; }
the comment after 'const' is bogus. We need the throw() to override the
virtual function.can  "nearbyint()" be renamed to nearby_int() ? (and you
don't need the quotes when it is already in <code>).About the traits, then I
would prefer the freestanding conventiontemplate<class Traits>
  struct YourRawConverterPolicy
  {
    typedef typename Traits::result_type   result_type   ;
    typedef typename Traits::argument_type argument_type ;
This would also mean some design changes, right? Didn't you talk about this
before the review? Also, can the two nested typedefs somehow be avoided. If
the policies is used heavily in generic code with expression templates, then
compile time matters and many nested typedefs is a killer.----did you plan
to make this into freestanding traits?class numeric_conversion_traits<>?

br

Thorsten