Re: [PATCH v3 03/28] Add new classes that model DWARF stack element

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

Hi Lancelot,

Thank you for reviewing my patches. I know that they are a bit big.

> I am far from having reached the end of your series.  I am not entirely
> sure if I should send comments as I progress or go through all of it and
> send all my comments at once when I have reached the end (which can take
> a while).  Is there a way you prefer?
> 
> Best,
> Lancelot.

Maybe it is better to do it as you progress because that way I can fix 
things in the same order on my side.

I will also let some time period to pass before I send the updated 
series, to give people time to comment on the current one.

>> +class dwarf_entry
>> +{
>> +public:
>> +  /* Not expected to be called on it's own.  */
>> +  dwarf_entry () = default;
> 
> If this constructor is not meant to be called directly, I guest it could
> be possible to make it protected.  This way it can no longer be used by
> a user of the class while still be usable by the child of this class.
> 

Good point.

>> +
>> +  virtual ~dwarf_entry () = default;
>> +};
>> +
>> +/* Location description entry found on a DWARF expression evaluation
>> +   stack.
>> +
>> +   Types of locations descirbed can be: register location, memory
>> +   location, implicit location, implicit pointer location, undefined
>> +   location and composite location (composed out of any of the location
>> +   types including another composite location).  */
>> +
>> +class dwarf_location : public dwarf_entry
>> +{
>> +public:
>> +  /* Not expected to be called on it's own.  */
>> +  dwarf_location (gdbarch *arch, LONGEST offset)
>> +    : m_arch (arch), m_offset (offset)
>> +  {}
> 
> Same remark on the visibility of the constructor here (based on the same
> comment).
> 

Agreed.

>> +
>> +  virtual ~dwarf_location () = default;
>> +
>> +  /* Add bit offset to the location description.  */
>> +  void add_bit_offset (LONGEST bit_offset)
>> +  {
>> +    LONGEST bit_total_offset = m_bit_suboffset + bit_offset;
>> +
>> +    m_offset += bit_total_offset / HOST_CHAR_BIT;
>> +    m_bit_suboffset = bit_total_offset % HOST_CHAR_BIT;
>> +  };
>> +
>> +  void set_initialised (bool initialised)
>> +  {
>> +    m_initialised = initialised;
>> +  };
>> +
>> +protected:
>> +  /* Architecture of the location.  */
>> +  gdbarch *m_arch;
>> +
>> +  /* Byte offset into the location.  */
>> +  LONGEST m_offset;
>> +
>> +  /* Bit suboffset of the last byte.  */
>> +  LONGEST m_bit_suboffset = 0;
>> +
>> +  /* Whether the location is initialized.  Used for non-standard
>> +     DW_OP_GNU_uninit operation.  */
>> +  bool m_initialised = true;
>> +};
>> +
>> +/* Value entry found on a DWARF expression evaluation stack.  */
>> +
>> +class dwarf_value : public dwarf_entry
> 
> It does not look like dwarf_value has child classes. It could be
> declared final as you have done for dwarf_undefined for example.

Agreed.

> 
>> +{
>> +public:
>> +  dwarf_value (gdb::array_view<const gdb_byte> contents, struct type *type)
>> +    : m_contents (contents.begin (), contents.end ()), m_type (type)
>> +  {}
>> +
>> +  dwarf_value (ULONGEST value, struct type *type)
>> +    : m_contents (TYPE_LENGTH (type)), m_type (type)
>> +  {
>> +    pack_unsigned_long (m_contents.data (), type, value);
>> +  }
>> +
>> +  dwarf_value (LONGEST value, struct type *type)
>> +    : m_contents (TYPE_LENGTH (type)), m_type (type)
>> +  {
>> +    pack_unsigned_long (m_contents.data (), type, value);
> 
> Shouldn't it be pack_long instead here?

It should, and it was in one of the previous patches. Copy paste mistake 
from my last iteration I guess.

> 
>> +  }
>> +
>> +  gdb::array_view<const gdb_byte> contents () const
>> +  {
>> +    return m_contents;
>> +  }
>> +
>> +  struct type* type () const
> 
> Minor remark first so I do not forget: I think GDB coding style prefers
> 'type *' over 'type* ' (space before the '*', no space after).

No matter how many times I go through these patches, I will still miss 
something...

Thanks :)

> 
> Unless there is a reason not to, I would be tempted to return a pointer
> to const (i.e. const struct type *) in order to maintain the const
> correctness here.
> 
> One way out could be to have two versions of the type method:
> 
>    struct type *type ();
>    const struct type *type () const;
> 
> I have tested this on top of the entire series, it seems to be OK. >

I am fine with this change as long as there is no existing API that 
prevents me to use the const type *, and it seems that you already 
checked for that.

I will make sure to fix everything in the next iteration.

Zoran