[boost] Re: Numeric Conversion Formal Review

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2003-12-12 16:11:50

Guillaume Melquiond wrote:
> Le lun 08/12/2003 à 13:04, Thorsten Ottosen a écrit :
>
> [...]
>
>>   a.. What is your evaluation of the design?
>
> I think the design is good.

Thanks

> The number of parameters of the converter
> template seems a bit high. In particular, was it really necessary to
> provide both a Float2IntRounder and a RawConverter parameters?
>
Choosing between different float-2-int rounding strategies is necessary only
when when doing this kind of conversion.

Choosing an alternative RawConverter is rarely neccesary, and when it is, it
is because of a UDT type which doesn't get along well with standard
conversions. In this situation, the custom RawConverter is required for all
sorts of conversions, not just float-2-int.

The two policies serve two different purposes: one is to let you choose how
to chop a float-type to get an int-type; the other is only to support
unconventional UDTs.
If the two were mixed into a single policy the work to adapt an UDT would be
harder. It wouldn't requires just a custom RawConverter.

> I also don't really understand the interest of having both an
> OverflowHandler and a UserRangeChecker::validate_range. I would simply
> remove this member since its only purpose is to use OverflowHandler
> (the converter could do that itself, no need for the user to provide
> it).
>
Again, the UserRangeChecker is intended to be used only for UDTs and when
you want to have some sort of range checking rather than none as it is by
default.
For conventional uses, there is no UserRangeChecker passed at all, which
enables the optimized range checking logic burried inside the
implementation.

OTOH, the OverflowHandler is needed in any case since you need to control
what to do in case of overflow whether you use the builtin range checking or
your own.

>
> And bounds::smallest could return 1 rather than 0 for integers.
>
Right.

>>   b.. What is your evaluation of the implementation?
>
> As I explained in other mails, there are a few shortcomings with the
> rounding from a floating-point value to an integer when rounding to
> nearest.
>
Yes, thanks... I'll see to fix that.

> Other than that it seems alright to me. But it is bit hard to verify a
> code with a design thought to be fully meta-programing-compliant :-).
>
<g> Tell me about it! :-)

> Could it be possible for the library to use another directory tree?
> The
> root directory boost/ is already a dumping ground, it's not a reason
> for
> the subdirectory boost/numeric/ to become one also :-). I would let
> only cast.hpp in this directory (so we can #include
> <boost/numeric/cast.hpp>)
> but the other files should be put in their own subdirectory.
>
I thought about it too.. the only concern is that if another subdir is used,
say, "conversions", then another namespace should be used too, which means
that the user code would look like:
boost::numeric::conversions::numeric_converter<...>(....)
Isn't it too long? Or should I use a new subdir but not another namespace?

>>   c.. What is your evaluation of the documentation?
>
> The documentation may benefit from a bit of tidying, in particular in
> the definitions. However, it is great to have such a big documentation
> for such a "small" library. That's a good thing.
>
Thanks!
It 's going to be even bigger though, a couple of things are missing.

>>   d.. What is your evaluation of the potential usefulness of the
>> library?
>
> This library is really useful. A few languages (e.g. Ada) always check
> that conversions are meaningful but C++ doesn't. Now it will be
> possible
> in a "standardized" way. I'm a bit surprised that lexical_cast was
> born before numeric_cast.
>
I think that the original numeric_cast<> was born before... I had actually
darfted this library
some years ago but just recently finished it for formal review.

>>   e.. Did you try to use the library?  With what compiler?  Did you
>> have any problems?
>
> I only tried with GCC 3.3. I didn't encounter any problems.
>
> On an architecture with an extended precision floating-point unit
> (namely x86), the converter_test will fail on d->f because of compiler
> optimizations (the use of a 10-byte floating-point temporary). It may
> be better to add a volatile keyword to neutralize it (I only tested
> with
> GCC but I suppose other compilers would probably behave similarly).
>
OK, noted.

> There was also a problem with udt_support_test at runtime.
>
Oh yes, I think I got this one.

>>   f.. How much effort did you put into your evaluation? A glance? A
>> quick reading? In-depth study?
>
> A few hours. The study was in-depth concerning floating-point behavior
> since it's my field.
>
And I thank you for that!

>>   g.. Are you knowledgeable about the problem domain?
>
> I'm knowledgeable about computer arithmetic. I'm not that
> knowledgeable
> when it comes to advanced C++ design.
>
>>   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.
>
> I think the library should be accepted: the design seems good to me
> and
> it will be a really useful addition.

Great!

>  However the implementation needs
> to
> be corrected first (or the documentation should explain its
> shortcomings).
>
I'll try to fix the implementation, but since the Round policy will get
somewhat complex, I will also stress in the documentation that this policy
is _not_ used by default (Trunc is), so the complexity will be there just
for those in need of using it.

Best,

Fernando Cacciola
SciSoft