Re: [boost] Ann: Floating Point Utilities Review starts today

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.