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

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

On Sat, 2008-01-05 at 13:30 +0200, Eli Zaretskii wrote:
> > Is this version ok?
> 
> Yes, but.  I took a look on the error messages you added, and found a
> few that need some fixing:

Ok.

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

Yes, that's what the libiberty implementation says. The Linux manpage is
somewhat vague, though: "If memory allocation wasn’t possible, or some
other error occurs, these functions will return -1". The glibc
implementation returns -1 for memory errors, or the return value of
vfprintf, the manpage of which says that "If an output error is
encountered, a negative value is returned.". So it seems that these
functions don't let the caller know exactly what led them to failure.

But it seems unlikely that an output error would happen when writing to
a string, so I guess we could assume that negative return values are
memory allocation errors. Then the error message could be changed to:

error (_("Error in memory allocation for conversion to decimal
float."));

> 
> > +	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?

Actually, it turns out this else block is unreachable in the current
code. All callers of value_args_as_decimal check if the types are either
decfloat or integral before calling it. I'm not sure if it's better to
leave the else part just in case the code changes, or remove it.

If the message is left there, it can be changed to:

error (_("Don't know how to convert from %s to %s."), TYPE_NAME (type1),
TYPE_NAME (type2));

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

I thought "Integer-only operation" already told the user that it's not
allowed on floating point number. But I changed it to:

error (_("Operation not valid for decimal floating point number."));

What do you think of the above?

> 
> > 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.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center