Re: [boost] Floating Point Utilities Review starts today

From: Guillaume Melquiond (guillaume.melquiond_at_[hidden])
Date: 2008-02-25 11:40:46

Le lundi 18 février 2008 à 17:27 +0000, John Maddock a écrit :
> The review of Johan Rade's floating point utilities starts today.

> The library consists of three parts:
> 
> 1) Floating point classification routines: these are optimised 
> implementations of the C99 and C++ TR1 functions fpclassify, isinf, isnan, 
> isnormal and isfinite.

Concerning isinf, I'm missing the feature of the glibc C library. Since
C99/Posix mandates an int as the return type (of the non-macro version),
the glibc returns 1 or -1 depending on the sign of the infinity. This is
quite nice, and compatible with the standard.

Due to the way the code is written (memcpy), it is compliant with the
following part of the C99 standard: "First, an argument represented in a
format wider than its semantic type is converted to its semantic type.
Then determination is based on the type of the argument." But it may
seem like a stroke of luck. So perhaps a big "careful!" comment should
be written inside the code, so that this property is not inadvertently
lost.

In the portability section of the documentation, please do not talk
about "IEEE 754 extended double precision with 15 exponent bits". It is
quite misleading. The standard actually defines a binary interchange
format that has a 15-bit exponent. Its official name is "binary128", its
common name is "quad(ruple)" precision. There are no longer things like
"extended double" formats (since "extended single" got scrapped). So if
you really mean binary128, please write it clearly.

The fp_traits.hpp file use BOOST_STATIC_CONSTANT (hence potentially an
enumeration) with constants that are not representable as int (0x800..).
I don't remember the precise details, but I know there is a disaster
bound to happen. I think a simple workaround is to just use 0x80000000u
instead (notice the "u"), but I can't vouch it is sufficient. As a
matter of fact, I would also mark the other big constants as unsigned at
line 345 and 349, just to be on the correct/safe side.

> 2) Sign manipulation functions: implementations of the C99 and C++ TR1 
> functions copysign and signbit, plus the changesign function.  Two of these 
> (signbit and copysign) are currently undocumented members of Boost.Math, and 
> again the two implementations will get merged if this library is accepted.

I am missing the point of the changesign function. What is its
rationale? The documentation talks about copysign(x, signbit(x) ? 1 :
-1) but not about -x. So I am a bit lost.

> 3) C++ locale facets: these will read and write non-finite numbers in a 
> portable and round-trippable way: that is not otherwise possible with 
> current C++ std library implementations.  These are particularly useful for 
> number-serialisation for example.

It may be worth writing somewhere in the documentation that the library
is also compliant with the IEEE 754R standard on floating-point
arithmetic. Indeed, as far as I can tell, the only missing features are
"should" features:

- a signaling NaN should produce, either "snan", or "nan" with invalid
signaled
- "snan" should be supported in input and it should produce, either a
signaling NaN, or a quiet NaN with invalid signaled

I have never been a strong believer in the greatness of signaling NaNs
anyway, so I don't really miss them.

Another missing feature is the exact round-trip, but one may argue that
this library is not the one to provide this mandatory feature.
Therefore, since it doesn't provide exact conversion, it doesn't have to
support output of NaN payloads. (Note that the round-trip, however, is
allowed to lose the signaling part of the encoding, as already written
above.)

What is the rationale for not enabling signed_zero by default?

Concerning the code, the put_impl function contains a lot of code
duplication. You should factor the code about the sign inside the
put_num_and_fill function. It would make the code simpler.

Best regards,

Guillaume