[boost] Re: Numeric Conversion Formal Review

From: Guillaume Melquiond (guillaume.melquiond_at_[hidden])
Date: 2003-12-12 17:57:00

Le ven 12/12/2003 à 22:11, Fernando Cacciola a écrit :
> Guillaume Melquiond wrote:

[...]

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

It doesn't seem that harder to me since you force the user to define
floor and ceil (in order to get conversions to integer). If she doesn't
want to, she needs to provide the two policies although in my opinion
only one should be needed (RawConverter).

But it's not that important and it's probably cleaner to have two
separate policies.

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

Yes, it was exactly my point. Since you expect OverflowHandler to be
used in both cases, you shouldn't ask the user to define the *member*
UserRangeChecker::validate_range since it is the function that uses
OverflowHandler. validate_range should be a member of converter, it
doesn't have anything to do with UserRangeChecker (since it already
provides the member out_of_range). Is my point clearer?

[...]

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

I don't have a strong opinion. Even if there isn't another namespace, I
still think it's clearer to have libraries in their own directories. Hi
people, any other opinion on this point?

[...]

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

Good point.

Regards,

Guillaume