Re: [PATCH 05/25] Use visitor pattern for "maint print c-tdesc"

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-06-12 10:41, Yao Qi wrote:
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index e2dcd1d..ac19792 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -35,6 +35,23 @@
>  #include "hashtab.h"
>  #include "inferior.h"
>
> +class tdesc_element_visitor
> +{
> +public:
> +  virtual void visit (const target_desc *e) = 0;
> +  virtual void visit_end (const target_desc *e) = 0;

For elements which children, and therefore can be visited before and 
after their children, I'd suggest using "visit_pre" and "visit_post" for 
the methods names.  It relates better to the terms pre-order and 
post-order.

> @@ -259,6 +287,30 @@ public:
>
>    /* The types associated with this feature.  */
>    VEC(tdesc_type_p) *types = NULL;
> +
> +  void accept (tdesc_element_visitor &v) const override
> +  {
> +    v.visit (this);
> +
> +    struct tdesc_type *type;
> +
> +    for (int ix = 0;
> +	 VEC_iterate (tdesc_type_p, types, ix, type);
> +	 ix++)
> +      {
> +	type->accept (v);
> +      }

No need for braces here (and for all the ->accept calls in for loops).

> +
> +    struct tdesc_reg *reg;
> +
> +    for (int ix = 0;
> +	 VEC_iterate (tdesc_reg_p, registers, ix, reg);
> +	 ix++)
> +      {
> +	reg->accept (v);
> +      }
> +  }
> +
>  } *tdesc_feature_p;
>  DEF_VEC_P(tdesc_feature_p);
>
> @@ -268,23 +320,67 @@ DEF_VEC_P(arch_p);
>
>  /* A target description.  */
>
> -struct target_desc
> +struct target_desc : tdesc_element
>  {

It's not a big deal, but the C++ification of target_desc would have 
fitted better in patch 03/25 I guess.

> +public:

You can remove public:.

> @@ -1707,279 +1783,288 @@ unset_tdesc_filename_cmd (char *args, int 
> from_tty)
>    target_find_description ();
>  }
>
> -static void
> -maint_print_c_tdesc_cmd (char *args, int from_tty)
> +class print_c_tdesc : public tdesc_element_visitor

IWBN to have "visitor" in the class name: print_c_tdesc_visitor.

>  {
> -  const struct target_desc *tdesc;
> -  const struct bfd_arch_info *compatible;
> -  const char *filename, *inp;
> -  char *function, *outp;
> -  struct property *prop;
> -  struct tdesc_feature *feature;
> -  struct tdesc_reg *reg;
> -  struct tdesc_type *type;
> -  struct tdesc_type_field *f;
> -  int ix, ix2, ix3;
> -  int printed_field_type = 0;
> +public:
> +  print_c_tdesc (std::string &filename_after_features)
> +    : m_filename_after_features (filename_after_features)

Could you document the parameter filename_after_features?

I didn't read the whole visitor code (it's getting late here...), but I 
like the idea and the pattern.

Simon