Re: [PATCH 5/9] Add support for printing value of DWARF-based fixed-point type objects

Simon Marchi
2020-11-14T13:20:19Z

On 2020-11-14 5:48 a.m., Joel Brobecker wrote:
> Hi Simon,
>
>>> + of the corresponding TYPE by setting its TYPE_FIXED_POINT_INFO.
>>> + CU is the DIE's CU. */
>>> +
>>> +static void
>>> +finish_fixed_point_type (struct type *type, struct die_info *die,
>>> + struct dwarf2_cu *cu)
>>
>> Unless there's a good reason not to (coming up in the following
>> patches), I would make this function create the "struct type", instead
>> of having the caller create it and pass it. In other words, this
>> signature:
>>
>> struct type *create_fixed_point_type (die_info *die, dwarf2_cu *cu)
>>
>> That just makes it more obvious that it's a simple "die" to "type"
>> transform.
>
> While implementing this suggestion, I'm realizing that doing so
> requires either:
>
> (1) Recomputing the following information for the CU: encoding,
> bitsize if any, and type name, which means:
>
> | attr = dwarf2_attr (die, DW_AT_encoding, cu);
> | if (attr != nullptr && attr->form_is_constant ())
> | encoding = attr->constant_value (0);
> | attr = dwarf2_attr (die, DW_AT_byte_size, cu);
> | if (attr != nullptr)
> | bits = attr->constant_value (0) * TARGET_CHAR_BIT;
> | name = dwarf2_name (die, cu);
> | if (!name)
> | complaint (_("DW_AT_name missing from DW_TAG_base_type"));
>
> Or:
>
> (2) Passing that information as arguments to the function.
>
> I find option (1) really sad because of the code duplication.
> And I find option (2) somewhat unsatisfactory, because we then
> run the risk of passing inconsistent information.
>
> This might explain partly why the init + finish approach isn't
> new in this unit...
>
> I'm attaching the patch which shows how option (1) looks like.
> For me, the current approach strikes a better balance between
> API cleanliness and implementation considerations. But I don't
> have a strong opinion against implementing what you suggest,
> including through option (2) (or other options I missed).
>
> Let me know what you think.
>
> --
> Joel
>

Ok I see, well that convinces me the original code is fine.

Simon