[boost] Re: Numeric Conversion Formal Review

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

Guillaume Melquiond wrote:
> 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).

Which aren't all the possible conversions.

>If she doesn't want to, she needs to provide the two policies
>
Only if udt->int is used

> although in my opinion only one should be needed (RawConverter).
>
In most cases, only one will be provided.

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

>>> 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?
>
Yes, I see your point now. Thanks.

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

OK, I agree.

Best,

Fernando Cacciola
SciSoft