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

[Simon Marchi <simark@simark.ca>]

On 2020-11-22 6:14 a.m., Joel Brobecker wrote:
> 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.

I also don't know how it would look if/when we convert this to a
hierarchy of classes, and I also prefer that we do what makes sense
today rather than act on speculations of what this could become.  We can
always change things later, it's not an API that we have to keep stable.

Following my changes, there are already methods in struct type that
apply to only some type codes, like bounds/set_gounds.  So I think it's
ok to make fixed_point_scaling_factor a method too, the same solution
will apply to all of them (deciding whether it's a virtual method only
implemented by some classes, or if it's a non-virtual method on some
specific classes).

In any case, I think the series looks good.  I let you decide if you
want to make fixed_point_scaling_factor a method or not, it doesn't
make a big difference in the end.

Simon