Re: [PATCH 3/7] Remove byte vectors from cplus_struct_type

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Tom

On Thu, Sep 21, 2023 at 12:01:30PM -0600, Tom Tromey via Gdb-patches wrote:
> This removes some byte vectors from cplus_struct_type, moving the
> information into bitfields in holes in struct field.
> ---
>  gdb/dwarf2/read.c |  86 +++++++-------------------------------
>  gdb/gdbtypes.c    |  53 -----------------------
>  gdb/gdbtypes.h    | 118 ++++++++++++++++++++++++---------------------------
>  gdb/stabsread.c   | 123 +++++++++++++++++++-----------------------------------
>  4 files changed, 113 insertions(+), 267 deletions(-)
> 
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 505c8ba12b5..c72512b8204 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -668,6 +668,46 @@ struct field
>      m_loc.dwarf_block = dwarf_block;
>    }
>  
> +  /* True if this field is 'private'.  */
> +  bool is_private () const
> +  { return m_private; }
> +
> +  /* Set the field's "private" flag.  */
> +  void set_private ()
> +  {
> +    /* Can't have both.  */
> +    gdb_assert (!m_protected);
> +    m_private = true;
> +  }
> +
> +  /* True if this field is 'protected'.  */
> +  bool is_protected () const
> +  { return m_protected; }
> +
> +  /* Set the field's "protected" flag.  */
> +  void set_protected ()
> +  {
> +    /* Can't have both.  */
> +    gdb_assert (!m_private);
> +    m_protected = true;
> +  }
> +
> +  /* True if this field is 'virtual'.  */
> +  bool is_virtual () const
> +  { return m_virtual; }
> +
> +  /* Set the field's "virtual" flag.  */
> +  void set_virtual ()
> +  { m_virtual = true; }
> +
> +  /* True if this field is 'ignored'.  */
> +  bool is_ignored () const
> +  { return m_ignored; }
> +
> +  /* Set the field's "ignored" flag.  */
> +  void set_ignored ()
> +  { m_ignored = true; }
> +
>    union field_location m_loc;
>  
>    /* * For a function or member type, this is 1 if the argument is
> @@ -677,6 +717,15 @@ struct field
>  
>    unsigned int m_artificial : 1;
>  
> +  /* Whether the field is 'private'.  */
> +  bool m_private : 1;
> +  /* Whether the field is 'protected'.  */
> +  bool m_protected : 1;

Is there a reason I miss to not use an enum to describe the visibility?
This would avoid having to verify that m_private and m_protected are
mutually exclusive.

Is seems to me that something similar to

    debug_visibility m_visibility : 2;

(or a custom enum) should work.

> +  /* Whether the field is 'virtual'.  */
> +  bool m_virtual : 1;
> +  /* Whether the field is 'ignored'.  */
> +  bool m_ignored : 1;
> +
>    /* * Discriminant for union field_location.  */
>  
>    ENUM_BITFIELD(field_loc_kind) m_loc_kind : 3;
> diff --git a/gdb/stabsread.c b/gdb/stabsread.c
> index 7402a26a401..9a84e5f3f80 100644
> --- a/gdb/stabsread.c
> +++ b/gdb/stabsread.c
> @@ -3108,11 +3115,15 @@ read_baseclasses (struct stab_field_info *fip, const char **pp,
>  	}
>        ++(*pp);
>  
> -      newobj->visibility = *(*pp)++;
> -      switch (newobj->visibility)
> +      int visibility = *(*pp)++;
> +      switch (visibility)
>  	{
>  	case VISIBILITY_PRIVATE:
> +	  newobj->field.set_private ();
> +	  break;
>  	case VISIBILITY_PROTECTED:
> +	  newobj->field.set_private ();

I think this should be:

  newobj->field.set_protected ();

> +	  break;
>  	case VISIBILITY_PUBLIC:
>  	  break;
>  	default:

Best,
Lancelot.