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

[Joel Brobecker <brobecker at adacore dot com>]

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