Re: [PATCH] Use visitors for make_gdb_type

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

Hi Alan,

besides the comments Simon already made, the patch looks fine to me.

Thanks
Philipp


On Sun, 28 Jan 2018 21:24:19 -0500
Simon Marchi <simark@simark.ca> wrote:

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