[boost] Re: Nuemric Conversion review

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2003-12-18 15:14:16

"Thorsten Ottosen" <nesotto_at_[hidden]> escribió en el mensaje
news:brs8i8$6qh$1_at_sea.gmane.org...
> 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.
>
Some traits are free-standing:

int_float_mixture<>
sign_mixture<>
udt_builtin_mixture<>
is_subranged<>

And there's also the bigger traits:

conversion_traits<> that includes all of the above plus some aliases and
additional useful types.

> I can't see if this will affect the desing of policy classes etc.
>
I think it won't.

>   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.
>
Yes, I'm working on this.

>   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.
>
Great!

> 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
>
AFAICT the probems with vc7.1 are actually MPL problems.

> (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
>
Yes, noted.

> -----------------------------
> 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.
>
The definition uses the term abstract "numeric value" rather than abstract
"number", but anyway...
An "abstract numeric number" is the term I've choosen to refer to just
numbers... that is, those mathematical entities called numbers that have
nothing to do -in themselves- with computer _representations_ of them.
ALL numbers as we _think_ of them are abstract numberic values in the
context of this library... including real, complex or whatever...
The term 'abstract' simply refers to an actual, purely mathematical,
'number' without any particular machine representation.

The counterpart of an "abstract numeric value", or just "number" if you
prefer (forget about abstract) is a representation of it in a particular
format. This is called a "typed numeric value".  A typed numeric value is
often only an _aproximation_ of its correspoding abstract numeric value.

> 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.
>
OK, "very very" large in the context of traditional machine-word-size
scales.
The idea is that if you need to count the number of cows in your farm you
don't need unsigned types no matter how big your farm is.
For example, the choice of an unsigned type for the size of a
_general_purpose_ container is right since _some_ container might have a
hugh amount of elements.

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

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

> 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?
>
The range checking is performed only when needed and only at the extent
actually needed.
In the case of T==S, the return value is a reference allowing most compilers
to effectively transform the "conversion" into nothing.
In the case S is a UDT the argument is taken by reference and nbot by value.

> Why is the return  type not just Target?
To allow a reference to be returned when T==S

> 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).
OK
>
"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.

Why? This form of documentation is used all over boost.

> can  "nearbyint()" be renamed to nearby_int() ?
"nearbyint" is the name of a C99 function.

> (and you don't need the quotes when it is already in <code>).
OK

> 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 ;

Hmmm... I don't get this.... can you explain?

> This would also mean some design changes, right? Didn't you talk about
this
> before the review?

I don't know :-)

> Also, can the two nested typedefs somehow be avoided.

Do you mean "result_type" and "argument_type"?
Remember that these are not necessarily just T and S, but can be "T const&"
and/or "S const&"

> 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?

Hmmm, what do you mean with "this"? What exactly did I planned to include?

Thanks,

Fernando Cacciola
SciSoft