Re: [RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format

Joel Brobecker
2017-10-09T16:30:27Z

On Tue, Sep 05, 2017 at 08:21:35PM +0200, Ulrich Weigand wrote:
> [RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format
>
> One of the few still remaining uses of DOUBLEST in GDB is the Ada front-end
> code that handles scaling of Ada fixed-point types. The target format for
> those types is some integer format; to convert those values to standard
> floating-point representation, that integer needs to be multiplied by a
> rational scale factor, given as a pair of numerator and denominator.
>
> To avoid having to deal with long integer arithmetic, the current Ada
> front-end code currently performs those scaling operations in host
> DOUBLEST arithmetic. To eliminate this use of DOUBLEST, this patch
> changes the front-end to instead perform those operations in the
> *target* floating-point format (chosing to use the target "long double").
>
> The implementation is mostly straight-forward, using value_cast and
> value_binop to perform the target operations.
>
> Scanning in the scale numerator and denominator is now done into
> a host "long long" instead of a DOUBLEST, which should be large
> enough to hold all possible values. (Otherwise, this can be replaced
> by target-format target_float_from_string operations as well.)
>
> Printing fixed-point types and values should be completely unchanges,
> using target_float_to_string with the same format strings as current code.
>
> Bye,
> Ulrich
>
> ChangeLog:
>
> * ada-lang.c (cast_to_fixed): Reimplement in target arithmetic.
> (cast_to_fixed): Likewise.
^^^^^^^^^^^^^
cast_from_fixed :)

> (ada_scaling_type): New function.
> (ada_delta): Return value instead of DOUBLEST. Perform target
> arithmetic instead of host arithmetic.
> (scaling_factor): Rename to ...
> (ada_scaling_factor) ... this. Return value instead of DOUBLEST.
> Perform target arithmetic instead of host arithmetic.

Maybe mention that we are making this function non-static too?

> (ada_fixed_to_float): Remove.
> (ada_float_to_fixed): Remove.
> * ada-lang.h (ada_fixed_to_float): Remove.
> (ada_float_to_fixed): Remove.
> (ada_delta): Return value instead of DOUBLEST.
> (ada_scaling_factor): Add prototype.
>
> * ada-typeprint.c: Include "target-float.h".
> (print_fixed_point_type): Perform target arithmetic instead of
> host arithmetic.
> * ada-valprint.c: Include "target-float.h".
> (ada_val_print_num): Perform target arithmetic instead of
> host arithmetic for fixed-point types.

Generally speaking, the patch looks good to me, and the only remark
I have is actually an trivial C++ question which you probably had
covered.

We have a fixed point tests in the testsuite (gdb.ada/fixed_points.exp),
so having it run and pass after your change should be a very good sanity
check on its own.

Thanks for doing that!

[...]
> Index: binutils-gdb/gdb/ada-typeprint.c
> ===================================================================
> --- binutils-gdb.orig/gdb/ada-typeprint.c
> +++ binutils-gdb/gdb/ada-typeprint.c
[...]
> @@ -360,16 +361,23 @@ print_enum_type (struct type *type, stru
> static void
> print_fixed_point_type (struct type *type, struct ui_file *stream)
> {
> - DOUBLEST delta = ada_delta (type);
> - DOUBLEST small = ada_fixed_to_float (type, 1);
> + struct value *delta = ada_delta (type);
> + struct value *small = ada_scaling_factor (type);
>
> - if (delta < 0.0)
> + if (delta == nullptr)
> fprintf_filtered (stream, "delta ??");
> else
> {
> - fprintf_filtered (stream, "delta %g", (double) delta);
> - if (delta != small)
> - fprintf_filtered (stream, " <'small = %g>", (double) small);
> + std::string str;
> + str = target_float_to_string (value_contents (delta),
> + value_type (delta), "%g");

This is a C++ question, really. Does it make any difference if you
declare the std::string first, and then only set its contents in
a second statement? I can't remember the details, but it has to do
with initialization vs assignment. I _really_ hope that the string
class is sufficiently well designed that the two are really equivalent
in practice?

--
Joel