Re: [PATCH v3 14/28] Add new computed struct value callback interface

[Zoran Zaric <zoran.zaric@amd.com>]

>> +
>> +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