From: John Maddock (john_at_[hidden])
Date: 2008-02-27 07:50:29
OK, finally(!), here comes my review of this library - purely as a potential
user, not as review manager.
First off the headline: I think the library should be accepted into Boost.
fpclassify
~~~~~~~~~~
Having already demonstated my ignorance of the aliasing rules, I'm quite
happy to accept this as is :-) There's little to say design wise, as it
follows existing practice, so as long as Johan is happy to see this merged
with my code that handles non-builtin floating point numbers (class types
like NTL::RR or mpfr_class etc) then I'm happy.
It's been suggested that isinf should indicate the sign of the infinity.
Personally I would prefer isinf etc to return a bool as per TR1 than return
+1 or -1 etc to indicate the sign as well (as per glibc) - we have other
functions for that purpose, and IMO a function name should indicate it's
purpose clearly - relying on the sign of the result of isinf seems like a
recipe for hard to maintain code to me... just my 2c worth.
Sign Manipulation
~~~~~~~~~~~~~~~~~
As these use the same core code as the fpclassify code, I'm happy with the
implementation. We've discussed changesign previously, so I won't labour
the point again :-)
There is one other useful function worth having IMO:
template <class T>
int sign(T x);
returns 0 (x is zero), +1 (x is > 0) or -1 (x is < 0). This is trivial to
implement (lot's of old C code - for example the Numerical Recipies stuff -
implement this as a helper macro), and there's an undocumented version
currently in Boost.Math. Is this worth integrating with Johan's code? I've
found it quite useful in Boost.Math from time to time.
Number Facets
~~~~~~~~~~~~~
I think these are a good idea, a few comments about the interface follow:
1) I assume that these:
const int legacy;
const int signed_zero;
const int trap_infinity;
const int trap_nan;
Should be declared "static" as well?
2) I'm not completely sure about the "legacy" flag: shouldn't the facet just
read in everything that it recognises by default? Or maybe we should flip
the meanings and "legacy" should become "strict_read" or "nolegacy" or
something - I'm not that keen on those names, but Boosters can usually come
up with something snappy when asked :-)
3) I think someone else mentioned that there is a lot of boilerplate code
that gets repeated when using these facets, so I think I'd like to see
something like:
std::locale get_nonfinite_num_locale(int flags = 0, std::locale base_locale
= std::locale());
which returns a locale containing the same instantiations of these facets
that a default locale would have for num_put and num_get, which is to say:
nonfinite_num_get<char>,
nonfinite_num_get<wchar_t>
nonfinite_num_put<char>
nonfinite_num_get<wchar_t>
Then we could simply write:
my_iostream.imbue(get_nonfinite_num_locale());
or
std::locale::global(get_nonfinite_num_locale());
and be done with it.
4) I think the flags are useful, but would be a lot nicer as iostream
manipulators - although I accept this may not work with lexical_cast :-(
BTW what was the issue that would require separate file compilation for
manipulators?
Overall nice job.
Regards, John.