Re: [PATCH] Use visitors for make_gdb_type

[Simon Marchi <simark@simark.ca>]

On 2018-01-26 10:30 AM, Alan Hayward wrote:
> I appear to still have email issues - previous post had the tabs stripped out.
> Hoping this version is ok. Apologies.

Hi Alan,

I was able to apply it correctly.
> This patch implements the suggestion in the review for
> [PATCH v2 6/8] Create xml from target descriptions.
> 
> Remove the make_gdb_type functions from the tdesc_type_ classes.
> Replace with a static make_gdb_type function that uses a element
> visitor called gdb_type_creator.
> 
> I've defined gdb_type_creator inside make_gdb_type because it shouldn't
> be needed outside the function.
> 
> The method of creating the types has not changed.
> 
> This patch will allow a future patch to commonise the tdesc_types with
> gdbserver without having to move any gdb_type functionality.
> 
> Alan.

make_gdb_type_union & co could be methods of gdb_type_creator and access gdbarch
directly.  I think it would make sense to have all the knowledge of the tdesc type
to gdb type conversion inside that class.

> -  type *make_gdb_type (struct gdbarch *gdbarch) const override
> +static type *
> +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype)
> +{
> +  class gdb_type_creator : public tdesc_element_visitor
>    {
> -    type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ());
> -    if (gdb_type != NULL)
> -      return gdb_type;
> +  public:
> +    gdb_type_creator (struct gdbarch *gdbarch)
> +      : m_gdbarch (gdbarch)
> +    {}
>  
> -    switch (this->kind)
> +    type *get_type ()
>      {
> -      case TDESC_TYPE_STRUCT:
> -	return make_gdb_type_struct (gdbarch);
> -      case TDESC_TYPE_UNION:
> -	return make_gdb_type_union (gdbarch);
> -      case TDESC_TYPE_FLAGS:
> -	return make_gdb_type_flags (gdbarch);
> -      case TDESC_TYPE_ENUM:
> -	return make_gdb_type_enum (gdbarch);
> +      return m_type;
>      }
>  
> -    internal_error (__FILE__, __LINE__,
> -		    "Type \"%s\" has an unknown kind %d",
> -		    this->name.c_str (), this->kind);
> +    void visit_pre (const target_desc *e)
> +    {}

I think we should have empty default implementations of these visit functions in the
base class, instead of having to declare them empty when unnecessary.  Maybe Yao had
a reason not to do this initially?

In any case, make sure to mark the overriden methods with the "override" keyword.  Using
clang:

/home/simark/src/binutils-gdb/gdb/target-descriptions.c:416:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
    void visit_pre (const target_desc *e)
         ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:48:16: note: overridden virtual function is here
  virtual void visit_pre (const target_desc *e) = 0;
               ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:419:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
    void visit_post (const target_desc *e)
         ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:49:16: note: overridden virtual function is here
  virtual void visit_post (const target_desc *e) = 0;
               ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:422:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
    void visit_pre (const tdesc_feature *e)
         ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:51:16: note: overridden virtual function is here
  virtual void visit_pre (const tdesc_feature *e) = 0;
               ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:425:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
    void visit_post (const tdesc_feature *e)
         ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:52:16: note: overridden virtual function is here
  virtual void visit_post (const tdesc_feature *e) = 0;
               ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:544:10: error: 'visit' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
    void visit (const tdesc_reg *reg)
         ^
/home/simark/src/binutils-gdb/gdb/target-descriptions.c:58:16: note: overridden virtual function is here
  virtual void visit (const tdesc_reg *e) = 0;
               ^

Otherwise, LGTM.

Thanks,

Simon