[boost] Thoughts on fixdec

From: Daryle Walker (dwalker07_at_[hidden])
Date: 2003-07-16 01:51:52

This is on version 2.2 that is dated on 5 July 2003.  I haven't read 
any messages since July 12th, so I haven't seen any other reviews yet.

I think this library should be accepted.  I have some suggestions for 
fixes and other changes.

1.	We have two math-related namespaces already.  The boost::math 
namespace leans to theoretical work and boost::numeric leans to 
hard-core computation.  You should probably pick one of those 
namespaces (numeric?) and possibly rename the class to "fixed_decimal". 
  Lots of the current free-functions that are specific to the state of 
your class could move to static-member functions.

2.	There is a <boost/limits.hpp> header to give deficient systems the 
std::numeric_limits class template (and pass through if <limits> 
exists).  You can use that and get rid of all your non-limits 
contingency code.

3.	Instead of playing around with determining the significand type, 
just #include <boost/cstdint.hpp>, use boost::intmax_t and be done with 
it.  With the static-member idea I gave in [1], the class would start 
off with:

class decimal
{
public:
     typedef ::boost::intmax_t  significand_type;
     typedef int                scale_type;
     //...
};

4.	What, no (safe-)Boolean conversion?

5.	The conversion from "int" needs to check for overflow.  The 
conversion from "double" needs to check also for underflow.  The 
conversion from strings needs to check also for bad formats.

6.	There should only be conversions from strings, and _no_ mixed 
operations with strings.  At least "int" and "double" is in the same 
family of types as "decimal", but strings aren't.  There should be no 
illusion that strings and numbers are generally compatible, so 
operations with strings need to be explicitly converted.

7.	Does the scale need to be first in all constructors?  If it can be 
placed second, then converting constructors can be made and most of the 
mixed operator functions can go away.

class decimal
{
public:
     //...
     decimal( intmax_t value = 0, int scale = 0 );
     decimal( uintmax_t value, int scale = 0 );

     decimal( long double value, int scale = 0, rounding_mode = 
default_rounding() );

     explicit decimal( char const *representation, int scale = 0, 
rounding_mode = default_rounding() );
     explicit decimal( wchar_t const *representation, int scale = 0, 
rounding_mode = default_rounding() );
     //...
};

(Yes, I know that some numeric purists [and GCC] hate "long double," 
but I want to maximize the capabilities.)

8.	The "round_down" and "round_up" functions don't seem clear enough.  
They could mean:

        round_towards_zero and round_fleeing_zero

                or

        round_towards_negative_infinity and round_towards_positive_infinity

I read later on, so I know you meant the first pair.  Maybe you can 
change the names, and add two more functions corresponding to the 
second pair.

class decimal
{
     //...
     enum remainder
     {
         zero, less, half, more
     };

     typedef void (*rounding_mode)( significand_type &, remainder, bool 
);

     static  rounding_mode  default_rounding();
     static  rounding_mode  default_rounding( rounding_mode );
     //...
};

void  round_towards_zero( decimal::significand_type &, 
decimal::remainder, bool );
void  round_fleeing_zero( decimal::significand_type &, 
decimal::remainder, bool );
void  round_nearest( decimal::significand_type &, decimal::remainder, 
bool );
void  round_bankers( decimal::significand_type &, decimal::remainder, 
bool );
void  round_ceiling( decimal::significand_type &, decimal::remainder, 
bool );
void  round_floor( decimal::significand_type &, decimal::remainder, 
bool );

9.	What, no "operator <<" nor "operator >>"?  (The shift would be a 
power of ten.)

10.	The class is no longer a template, but some of the wording in the 
code and docs act like it's still a template.  (When you're rewording, 
there's some HTML errors to fix.)

11.	[This is general speculation.]  Maybe all numeric types should have 
member functions that change an object to its inverse inline.  Examples:

class MyNum
{
public:
     //...
     MyNum &  negate();       // additive inverse
     MyNum &  reciprocate();  // multiplicative inverse
     MyNum &  complement();   // bit(like) flip
     MyNum &  conjugate();    // complex (or above) conjugation
     //...
};

This would save time to do these operations without extra copying:

        "negate" eliminates "x = -x"
     with "reciprocate": "x / y" -> "x * y.reciprocate()"
        "complement" eliminates "x = ~x"
     "conjugate" eliminates "x = conj(x)"

Each operation would only be defined if it's appropriate.  Types 
without additive inverses don't get "negate".  The "reciprocate" 
operation requires EXACT (not approximate nor quotient & remainder) 
multiplicative inverses, like rational or modulo types.  The 
"complement" needs something supporting "operator ~()".  The 
"conjugate" operation would use complex types, or a higher level type 
(like quaterions).  You could define it for real number types, if you 
don't mind a no-op.

Getting back on-topic, "decimal" would only have "negate".  (As I said, 
"conjugate" could be done.)

Also, ...

class MyNum
{
public:
     //...
     bool  is_negative() const;
     //...
};

... would save time over doing a (possible) conversion from zero and a 
comparison.  This can give

short sgn( MyNum x ) { return x ? (x.is_negative() ? -1 : +1) : 0; }
MyNum abs( MyNum x ) { return x.is_negative() ? x.negate() : x; }

And ...

class MyNum
{
public:
     //...
     static std::pair<MyNum, MyNum>  div( MyNum dividend, MyNum divisor 
);
     //...
};

... would provide any easy default place to locate this operation if we 
ever put up a general "div" template.  The result is the quotient and 
remainder, in that order, of course.

Let's get back on-topic:

12.	We got a similar controversy during the Boost.Format review.  
Overriding the I/O functions to give custom results hoping that next 
step is the required canonical step is a bad idea.  Some of the custom 
formatting flags use an "iword" result.  The corresponding "pword" 
result can be used to store the current rounding function.  Code 
pointers and data pointers aren't guaranteed to be the same size, so 
dynamic allocation may be needed (over a reinterpret_cast).

13.	You have strange (regular) assignment semantics.  You try to change 
the receiver's logical value to the source's logical value, but retain 
the receiver's original scale (internally shifting the new significand 
and doing any rounding if needed).  Maybe you should use the automatic 
default of complete-splatter semantics and use special assignment 
methods for what you're using now.  Otherwise, you will get strange 
effects if you try to change containers of "decimal" through assignment.

class decimal
{
public:
     //...
     decimal &  assign( decimal const & );
       // regular "splattering" semantics

     decimal &  assign_retaining_scale( decimal const & );
       // maintains logical value, keeps old scale

     //...
};

(Note that this means that your member data can't be "const".  With 
that change, add operators "<<=" and ">>=" to [9].)

14.	If you have a "scale" member function to return the current 
object's scale; you should have something like a "significand" member 
function to return the object's mantissa.  This will allow copies of 
objects that retain just one aspect of their source.

15.	It took me a while to figure the following out, so maybe you should 
put it in simpler language.  Objects of this type retain a copy of the 
given value, rounded (via the given mode) and kept to a precision of 
the given scale number of (base-ten) fractional digits.  The value is 
stored as that scale and the (integral) number of 10**(-scale) units 
needed to add up to the value.

I guess that the scale needs to be non-negative?

16.	I don't understand all of the "[]" rounding operations, but the 
"[]" is bound to the object on the left (by C++'s parsing rules), and 
not to the operator.  You just adjust the spacing and wish that the 
brackets were associated with the operator on the right.  There's 
nothing wrong with your wish, except that most the documentation is 
worded as if that wish is what is happening.  Maybe you should weaken 
that assertion, since it isn't technically true.

17.	Do you really need to support VC++5 and (other?) systems with 
pre-standard I/O & Locales?  I don't bother with pre-standard 
compatibility with the I/O objects I contribute.

Daryle