Re: [patch 2/2] Wrap-up expression support for DFP.

Eli Zaretskii
2008-01-05T11:30:20Z

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> 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.