From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30447 invoked by alias); 5 Jan 2008 11:30:20 -0000 Received: (qmail 30439 invoked by uid 22791); 5 Jan 2008 11:30:20 -0000 X-Spam-Check-By: sourceware.org Received: from heller.inter.net.il (HELO heller.inter.net.il) (213.8.233.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 05 Jan 2008 11:30:00 +0000 Received: from HOME-C4E4A596F7 (IGLD-80-230-71-78.inter.net.il [80.230.71.78]) by heller.inter.net.il (MOS 3.7.3a-GA) with ESMTP id EOF18351 (AUTH halo1); Sat, 5 Jan 2008 13:29:56 +0200 (IST) Date: Sat, 05 Jan 2008 11:30:00 -0000 Message-Id: From: Eli Zaretskii To: Thiago Jung Bauermann CC: gdb-patches@sourceware.org In-reply-to: <1199304046.12907.77.camel@localhost.localdomain> (message from Thiago Jung Bauermann on Wed, 02 Jan 2008 18:00:46 -0200) Subject: Re: [patch 2/2] Wrap-up expression support for DFP. Reply-to: Eli Zaretskii References: <20071220054926.148275471@br.ibm.com> <20071220055107.194393592@br.ibm.com> <1198705277.12907.39.camel@localhost.localdomain> <1199304046.12907.77.camel@localhost.localdomain> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-01/txt/msg00073.txt.bz2 > From: Thiago Jung Bauermann > Cc: gdb-patches@sourceware.org > Date: Wed, 02 Jan 2008 18:00:46 -0200 > > Fine by me. Thanks for your review, Eli. The attached patch has the > following changes from the one I originally posted: > > - includes documentation update to GDB manual, under the "C and C++" > subsection; > - uses asprintf instead of sprintf, with "%.30Lg" in > decimal_from_floating; > - uses strtod instead of atof in decimal_to_double; > - decimal/binary FP conversion tests now accept some rounding variation: > > gdb_test "p (_Decimal64) 3.1" " = 3.(0999.*|1000.*)" > > > Is this version ok? Yes, but. I took a look on the error messages you added, and found a few that need some fixing: > + ret = asprintf (&buffer, "%.30Lg", value_as_double (from)); > + if (ret == -1) > + error (_("Error in conversion to decimal float.")); The documentation of asprintf says that -1 is returned if it fails to allocate memory for the buffer. So I think our error message should say the same. In any case, "error in conversion" is too vague to be useful. > + error (_("Unknown decimal floating point operation.")); Shouldn't this be internal_error? I mean, there couldn't be any valid op at this point, so this is a kind-of "can't happen" situation, isn't it? > + error (_("Don't know how to convert to decimal floating type.")); Wouldn't it be better to state the source type (from which we tried to convert) here as well? > + error (_("Integer-only operation on floating point number.")); Did you mean to say "Integer-only operation on floating point number is not allowed."? Or something else? As a GDB user, I'd be quite confused about the actual problem if I were to see this message. > Index: src-git/gdb/doc/gdb.texinfo > =================================================================== > --- src-git.orig/gdb/doc/gdb.texinfo 2007-12-28 01:11:37.000000000 -0200 > +++ src-git/gdb/doc/gdb.texinfo 2008-01-02 14:14:48.000000000 -0200 This part of your patch is approved. Thanks.