RFA v2: Various enhancements to the fixed-point support implementation

Joel Brobecker brobecker@adacore.com
Sun Nov 22 11:14:00 GMT 2020


Hello,

This is v2 of the following patch series:

  - [RFA v2 1/6] change and rename gmp_string_asprintf to return an
  - [RFA v2 2/6] gmp-utils: Convert the read/write methods to using
  - [RFA v2 3/6] gdbtypes.h: Get rid of the TYPE_FIXED_POINT_INFO macro
  - [RFA v2 4/6] Make fixed_point_type_base_type a method of struct type
  - [RFA v2 5/6] Make function fixed_point_scaling_factor a method of
  - [RFA v2 6/6] valarith.c: Replace INIT_VAL_WITH_FIXED_POINT_VAL macro

The only changes I made were to patch #1 and patch #2, following
the commends made by Simon and Pedro:

  - [RFA v2 1/6] change and rename gmp_string_asprintf to return an

        Rename the function to gmp_string_printf as recommended,
        and use the suggestion to rely on gmp_vsnprintf to only
        allocate the string buffer once.

  - [RFA v2 2/6] gmp-utils: Convert the read/write methods to using

        Improve the patch using either the form {ptr, len} to construct
        the gdb_byte array, or else gdb::make_array_view.

The other patches are just plain rebases.

Each patch was individually tested on x86_64-linux, with no regression.

There are still a number of comments which were made by Simon and
Pedro (xfail for m2 and pascal, NEWS entries, selftest error
using Ubuntu's GMP, etc), and I will address those next, in
separate submissions.

Note that I almost dropped patch #5 (turning fixed_point_scaling_factor
from function to method), since Simon replied to the discussion
about this topic that the the current form (a separate function) was fine.
In particular, I tried to think about it in a possible future context
where types are a tree of classes rather than one class with a type-code.
In that situation, the fixed_point_scaling_factor method would logically
be "attached" to the fixed point type class. This in turn means that
ranges of fixed point types would not have this method, forcing users
to have to call fixed_point_type_base_type first before they can query
the fixed_point_scaling_factor method. In other words, because
the functionality applies to two types where one is not the child of
the other (in terms of class inheritance), a functioned did seem better.

However, one could argue that the fixed_point_type_base_type method
is in exactly the same boat: It applies to both range types and
fixed point. So, in the name of being consistent, this patch series
keeps them both. My thinking process is based on a hypothetical
scenario, which I'm not even sure ever got discussed here anyways.

That being said, now that I write my thoughts, for the case of
fixed_point_type_base_type, if we did have a hierarchy of classes,
I could perhaps see how all types would have a "base_type" method
whose default implementation would simply return the type itself,
and were range types would return the target type. In that scenario,
the two methods are no longer in the same boat, and we can think
of dropping the fixed_point_scaling_factor patch if we wanted to.

I do not have a strong opinion and thus can easily drop any patch.
The reason I kept the change in the end is that it just seemed
more logical to act on what is today, rather than decide based on
what the code might be tomorrow.

Let me know what you guys think.

Thanks,
-- 
Joel


More information about the Gdb-patches mailing list