Re: [Qemu-devel] [PATCH V2] softfloat: Rename float*_is_nan() functions to float*_is_quiet_nan()

[Peter Maydell <peter.maydell@linaro.org>]

On 18 December 2010 11:49, Andreas Färber <andreas.faerber@web.de> wrote:
> IMO a lot of code in QEMU is cryptic because someone thinks that someone
> else must've thought something particular when doing it that way and is thus
> reluctant to touch it...

> For a fact, [u]int8 und [u]int64 remain unchanged width-wise.
> For [u]int16, only malc may know what that maps to on AIX, for which they
> are #ifndef'ed out. I doubt it's an int.
> Unless there's an ILP64 platform we support, [u]int32 would stay the same
> width, too.
> That's why I was saying, putting, e.g., a 33rd bit into int32 has undefined
> semantics, just as for the POSIX int_least32_t type. I don't see a win in
> declaring that information.

It's saying "it's OK to provide more than 32 bits if that would be faster",
and indeed that's exactly how we typedef it. Some parts of softfloat
that use bits32 do rely on there being exactly 32 bits and not 64.

> My patch tries to do three things in one:

> 1.) Fix mismatches between headers and sources, i.e. float32
> int32_to_float32(int); vs. float32 int32_to_float32(int32) {...} etc.

In this case we do know the rationale for this mismatch:
http://www.jhauser.us/arithmetic/SoftFloat-2b/SoftFloat-source.txt
---begin---
Unlike the actual function definitions in `softfloat.c', the declarations
in `softfloat.h' do not use any of the types defined by the `processors'
header file.  This is done so that clients will not have to include the
`processors' header file in order to use SoftFloat.  Nevertheless, the
target-specific declarations in `softfloat.h' must match what `softfloat.c'
expects.  For example, if `int32' is defined as `int' in the `processors'
header file, then in `softfloat.h' the output of `float32_to_int32' should
be stated as `int', although in `softfloat.c' it is given in target-
independent form as `int32'.
---endit---

...which doesn't really apply to the way qemu has taken softfloat,
so I agree it makes sense to change things here.

> 2.) Drop the unnecessary custom integer types in favor of standard ones.

...but it doesn't do this because it isn't touching the 'bits32' types which
are the ones which are exact synonyms for the posix int32_t &c.
If your aim is to remove unnecessary custom types this is the first and
easiest target because you can do it as a pure search-and-replace.

> 3.) Fix instances of lazyness where _t was forgotten and the mistake was
> hidden by the softfloat typedefs.
>
> Renaming int32 to qint32 would defeat the second purpose. I got around the
> Haiku issue for now, so that's not a pressing need.
>
> Had the softfloat code not been a real refactoring-unfriendly mess of int,
> int* and int*_t, I would've offered to do this in multiple steps per type. I
> could try splitting out part 1 above. Part 3 can easily be split off by
> cut-and-paste and could be applied independently.

I certainly think it would be easier to review as separate patches which
did different things.

> Promoting int16[_t] to int for things like shift counts is beyond the scope
> of my patch.

...but your patch changes what is currently an 'int' (hidden behind the int16
typedef) to int16_t, so it is making a change here. It's a safe change but
it doesn't actually make any sense.

-- PMM