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
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
