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.
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.
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.
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
std::locale get_nonfinite_num_locale(int flags = 0, std::locale base_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:
Then we could simply write:
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
Overall nice job.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk