[PATCH v3 14/28] Add new computed struct value callback interface
Zoran Zaric
zoran.zaric@amd.com
Thu Nov 4 11:32:32 GMT 2021
>> +
>> +class computed_closure : public refcounted_object
>> +{
>> +public:
>> + computed_closure (std::unique_ptr<dwarf_location> location,
>> + struct frame_id frame_id)
>> + : m_location (std::move (location)), m_frame_id (frame_id)
>> + {}
>> +
>> + computed_closure (std::unique_ptr<dwarf_location> location,
>> + struct frame_info *frame)
>> + : m_location (std::move (location)), m_frame (frame)
>> + {}
>> +
>> + const dwarf_location &get_location () const;
>
> Is there a reason not to inline this function definition as well? It
> seems as trivial as get_frame_id and get_frame.
>
There was a reason in one of the previous versions, but not anymore.
Thank you.
>> +
>> + frame_id get_frame_id () const
>> + {
>
> If I understand correctly, m_frame_id and m_frame are mutually
> exclusive. To be on the safe side, I would be tempted to add a check
> here:
>
> gdb_assert (m_frame == nullptr);
>
I didn't want to force them to be mutual exclusive, but when I think
about it more it makes sense to make them. Thanks.
>> + return m_frame_id;
>> + }
>> +
>> + frame_info *get_frame () const
>> + {
>> + return m_frame;
>> + }
>> +
>> +private:
>> + /* Entry that this class encloses. */
>> + const std::unique_ptr<const dwarf_location> m_location;
>> +
>> + /* Frame ID context of the closure. */
>> + frame_id m_frame_id;
>> +
>> + /* In the case of frame expression evaluator the frame_id
>> + is not safe to use because the frame itself is being built.
>> + Only in these cases we set and use frame info directly. */
>> + frame_info *m_frame = NULL;
>
> I guess nullptr is prefered over NULL (across the patch).
>
Thanks. I will do another pass on those.
>> +static void
>> +read_closure_value (value *v)
>> +{
>> + rw_closure_value (v, NULL);
>> +}
>> +
>> +static void
>> +write_closure_value (value *to, value *from)
>> +{
>> + rw_closure_value (to, from);
>> +}
>> +
>> +/* Check if a closure value V contains describes any piece
>
> One of 'contains' and 'describes' might be too much here I guess.
>
> Best,
> Lancelot.
Thank you,
Zoran
More information about the Gdb-patches
mailing list