Re: [boost] Floating Point Utilities Review starts today

From: Johan Råde (rade_at_[hidden])
Date: 2008-02-25 12:25:38

Guillaume Melquiond wrote:

> 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.

I'll have to think about that.
Spontaneously I'm a bit sceptical of functions
that do several different things at the same time.

> 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.

I don't understand. I'm not a C++ guru.
Could you please elaborate.

> 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.

I'll change that.

> 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.

I'll change that.

> 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.

Is -x guaranteed to change the sign of zero and NaN?
If it is, then I agree that the function is superfluous.

> 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.

I'm not a big fan of signaling NaN either.
There is the additional complication that the binary representation
of quiet/signaling NaN is not standardized.
For instance, a bit pattern that represents a quiet NaN on an Intel or PowerPC
processor, would represent a signaling NaN on a MIPS processor.
Therefore it would be tricky to provide a portable implementation.

> 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.)

I have no plans to implement that.

> What is the rationale for not enabling signed_zero by default?

I think most users of the library will not want it enabled by default.
They may not care much about the fine details of floating point arithmetic.
Insted they may just want to prevent crashes when deserializing text archives
that contain (by design or by accident) infinity or NaN.

> 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.

I will look into that.

Thank you for your detailed comments!

Johan Råde