From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56564 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PTvgn-0000oQ-Kh for qemu-devel@nongnu.org; Sat, 18 Dec 2010 07:15:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PTvgm-0000un-4j for qemu-devel@nongnu.org; Sat, 18 Dec 2010 07:15:05 -0500 Received: from mail-qy0-f173.google.com ([209.85.216.173]:45195) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PTvgm-0000ub-2C for qemu-devel@nongnu.org; Sat, 18 Dec 2010 07:15:04 -0500 Received: by qyk1 with SMTP id 1so2379408qyk.4 for ; Sat, 18 Dec 2010 04:15:03 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <399990E2-021F-40D3-8787-542379592760@web.de> References: <1292601366-990-1-git-send-email-peter.maydell@linaro.org> <5C19C2AC-87D5-4C9E-8D8D-E2FEC4AC89F5@web.de> <20101218023011.GN25059@nightcrawler> <399990E2-021F-40D3-8787-542379592760@web.de> Date: Sat, 18 Dec 2010 12:15:03 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH V2] softfloat: Rename float*_is_nan() functions to float*_is_quiet_nan() From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Andreas_F=C3=A4rber?= Cc: "Edgar E. Iglesias" , QEMU Developers , Nathan Froyd On 18 December 2010 11:49, Andreas F=C3=A4rber wro= te: > 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 t= hus > 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 undefin= ed > 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 whic= h 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 th= e > 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 sco= pe > of my patch. ...but your patch changes what is currently an 'int' (hidden behind the int= 16 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