Move TYPE_CODE_INTERNAL_FUNCTION type printing to common code (was: Re: [PATCH 2/6] Make "ptype INTERNAL_FUNCTION" in Ada print like other languages)

  On 2023-02-15 4:56 p.m., Tom Tromey wrote:
>>> I think this would be better.
> 
> Pedro> My point with adding this check early is that these functions' type never
> Pedro> has anything to do with Ada, so all that code at the beginning of ada_print_type,
> Pedro> like decoded_type_name, ada_is_aligner_type, ada_is_constrained_packed_array_type,
> Pedro> etc. is always a nop for internal functions, so might as well skip it all.
> 
> I see.  That makes sense to me, thanks.
> 
> Sorry about the noise, as far as I'm concerned you can land either
> version of the patch.

No problem.  I've merged the original version, along with the whole series.

> 
> Pedro> I actually started out by considering moving TYPE_CODE_INTERNAL_FUNCTION printing
> Pedro> to common code (say, rename the virtual language_defn::print_type to do_print_type,
> Pedro> and add a new non-virtual wrapper language_defn::print_type method and print
> Pedro> TYPE_CODE_INTERNAL_FUNCTION there), and I didn't pursue that as I couldn't really convince
> Pedro> myself that a different language might want to print it differently (say, some other
> Pedro> characters instead of "<>")
> 
> I think this is something we can simply force on languages.  The type of
> these things isn't a language feature and probably isn't worth
> customizing anyway.
> 
> More generally I think it's better for gdb to try to do more in the
> common code and leave less to the languages.

I implemented the idea I mentioned above.

WDYT?

From 73958ac0ee47fd1a887515a47436cb3835514724 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 15 Feb 2023 20:19:10 +0000
Subject: [PATCH] Move TYPE_CODE_INTERNAL_FUNCTION type printing to common code

Internal functions should print the same for all languages.  It is
currently left to each language to make sure that is true.  This patch
centralizes the logic.

Rename language_defn::print_type to language_defn::do_print_type and
making it protected, and then add a new public non-virtual
language_defn::print_type method that implements
TYPE_CODE_INTERNAL_FUNCTION type printing before dispatching to the
virtual language_defn::do_print_type.

Change-Id: Idd5fbb437cc91990b051e1a9a027c3909c09dd31
---
 gdb/ada-lang.c      |  6 +++---
 gdb/ada-typeprint.c |  7 -------
 gdb/c-lang.c        | 24 ++++++++++++------------
 gdb/d-lang.c        |  6 +++---
 gdb/f-lang.h        |  6 +++---
 gdb/f-typeprint.c   |  6 +++---
 gdb/go-lang.h       |  6 +++---
 gdb/go-typeprint.c  |  6 +++---
 gdb/language.c      | 25 ++++++++++++++++++++++---
 gdb/language.h      | 21 +++++++++++++++++----
 gdb/m2-lang.h       |  6 +++---
 gdb/objc-lang.c     |  6 +++---
 gdb/opencl-lang.c   |  6 +++---
 gdb/p-lang.h        |  6 +++---
 gdb/p-typeprint.c   |  6 +++---
 gdb/rust-lang.c     |  6 +++---
 gdb/rust-lang.h     |  6 +++---
 17 files changed, 90 insertions(+), 65 deletions(-)


base-commit: 141cd158423a5ee248245fb2075fd2e5a580cff2
  

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-15 4:56 p.m., Tom Tromey wrote:
>>>> I think this would be better.
>> 
>> Pedro> My point with adding this check early is that these functions' type never
>> Pedro> has anything to do with Ada, so all that code at the beginning of ada_print_type,
>> Pedro> like decoded_type_name, ada_is_aligner_type, ada_is_constrained_packed_array_type,
>> Pedro> etc. is always a nop for internal functions, so might as well skip it all.
>> 
>> I see.  That makes sense to me, thanks.
>> 
>> Sorry about the noise, as far as I'm concerned you can land either
>> version of the patch.
>
> No problem.  I've merged the original version, along with the whole series.
>
>> 
>> Pedro> I actually started out by considering moving TYPE_CODE_INTERNAL_FUNCTION printing
>> Pedro> to common code (say, rename the virtual language_defn::print_type to do_print_type,
>> Pedro> and add a new non-virtual wrapper language_defn::print_type method and print
>> Pedro> TYPE_CODE_INTERNAL_FUNCTION there), and I didn't pursue that as I couldn't really convince
>> Pedro> myself that a different language might want to print it differently (say, some other
>> Pedro> characters instead of "<>")
>> 
>> I think this is something we can simply force on languages.  The type of
>> these things isn't a language feature and probably isn't worth
>> customizing anyway.
>> 
>> More generally I think it's better for gdb to try to do more in the
>> common code and leave less to the languages.
>
> I implemented the idea I mentioned above.
>
> WDYT?
>
> From 73958ac0ee47fd1a887515a47436cb3835514724 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 15 Feb 2023 20:19:10 +0000
> Subject: [PATCH] Move TYPE_CODE_INTERNAL_FUNCTION type printing to common code
>
> Internal functions should print the same for all languages.  It is
> currently left to each language to make sure that is true.  This patch
> centralizes the logic.
>
> Rename language_defn::print_type to language_defn::do_print_type and
> making it protected, and then add a new public non-virtual
> language_defn::print_type method that implements
> TYPE_CODE_INTERNAL_FUNCTION type printing before dispatching to the
> virtual language_defn::do_print_type.

Looks like a good change to me.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Change-Id: Idd5fbb437cc91990b051e1a9a027c3909c09dd31
> ---
>  gdb/ada-lang.c      |  6 +++---
>  gdb/ada-typeprint.c |  7 -------
>  gdb/c-lang.c        | 24 ++++++++++++------------
>  gdb/d-lang.c        |  6 +++---
>  gdb/f-lang.h        |  6 +++---
>  gdb/f-typeprint.c   |  6 +++---
>  gdb/go-lang.h       |  6 +++---
>  gdb/go-typeprint.c  |  6 +++---
>  gdb/language.c      | 25 ++++++++++++++++++++++---
>  gdb/language.h      | 21 +++++++++++++++++----
>  gdb/m2-lang.h       |  6 +++---
>  gdb/objc-lang.c     |  6 +++---
>  gdb/opencl-lang.c   |  6 +++---
>  gdb/p-lang.h        |  6 +++---
>  gdb/p-typeprint.c   |  6 +++---
>  gdb/rust-lang.c     |  6 +++---
>  gdb/rust-lang.h     |  6 +++---
>  17 files changed, 90 insertions(+), 65 deletions(-)
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index ec85729042f..e3bbfdeae73 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -13625,9 +13625,9 @@ class ada_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      ada_print_type (type, varstring, stream, show, level, flags);
>    }
> diff --git a/gdb/ada-typeprint.c b/gdb/ada-typeprint.c
> index 3866b2d35eb..e95034c9285 100644
> --- a/gdb/ada-typeprint.c
> +++ b/gdb/ada-typeprint.c
> @@ -941,13 +941,6 @@ ada_print_type (struct type *type0, const char *varstring,
>  		struct ui_file *stream, int show, int level,
>  		const struct type_print_options *flags)
>  {
> -  if (type0->code () == TYPE_CODE_INTERNAL_FUNCTION)
> -    {
> -      c_print_type (type0, "", stream, show, level,
> -		    language_ada, flags);
> -      return;
> -    }
> -
>    struct type *type = ada_check_typedef (ada_get_base_type (type0));
>    /* If we can decode the original type name, use it.  However, there
>       are cases where the original type is an internally-generated type
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index 6535ab93498..9832d676445 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -836,9 +836,9 @@ class c_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> @@ -993,9 +993,9 @@ class cplus_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> @@ -1100,9 +1100,9 @@ class asm_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> @@ -1159,9 +1159,9 @@ class minimal_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> diff --git a/gdb/d-lang.c b/gdb/d-lang.c
> index 8286c5be646..a374fcfeb8f 100644
> --- a/gdb/d-lang.c
> +++ b/gdb/d-lang.c
> @@ -151,9 +151,9 @@ class d_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> diff --git a/gdb/f-lang.h b/gdb/f-lang.h
> index 673e273d31a..7c44a46b981 100644
> --- a/gdb/f-lang.h
> +++ b/gdb/f-lang.h
> @@ -87,9 +87,9 @@ class f_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override;
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override;
>  
>    /* See language.h.  This just returns default set of word break
>       characters but with the modules separator `::' removed.  */
> diff --git a/gdb/f-typeprint.c b/gdb/f-typeprint.c
> index e4aed6e5904..b3c675e2c2a 100644
> --- a/gdb/f-typeprint.c
> +++ b/gdb/f-typeprint.c
> @@ -46,9 +46,9 @@ f_language::print_typedef (struct type *type, struct symbol *new_symbol,
>  /* See f-lang.h.  */
>  
>  void
> -f_language::print_type (struct type *type, const char *varstring,
> -			struct ui_file *stream, int show, int level,
> -			const struct type_print_options *flags) const
> +f_language::do_print_type (struct type *type, const char *varstring,
> +			   struct ui_file *stream, int show, int level,
> +			   const struct type_print_options *flags) const
>  {
>    enum type_code code;
>  
> diff --git a/gdb/go-lang.h b/gdb/go-lang.h
> index 1820b4c9658..df607d053f9 100644
> --- a/gdb/go-lang.h
> +++ b/gdb/go-lang.h
> @@ -110,9 +110,9 @@ class go_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override;
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override;
>  
>    /* See language.h.  */
>  
> diff --git a/gdb/go-typeprint.c b/gdb/go-typeprint.c
> index 0c4e9a26563..c664aaa8284 100644
> --- a/gdb/go-typeprint.c
> +++ b/gdb/go-typeprint.c
> @@ -42,9 +42,9 @@
>     LEVEL indicates level of recursion (for nested definitions).  */
>  
>  void
> -go_language::print_type (struct type *type, const char *varstring,
> -			 struct ui_file *stream, int show, int level,
> -			 const struct type_print_options *flags) const
> +go_language::do_print_type (struct type *type, const char *varstring,
> +			    struct ui_file *stream, int show, int level,
> +			    const struct type_print_options *flags) const
>  {
>    /* Borrowed from c-typeprint.c.  */
>    if (show > 0)
> diff --git a/gdb/language.c b/gdb/language.c
> index 50a53c647f5..23601c42ad6 100644
> --- a/gdb/language.c
> +++ b/gdb/language.c
> @@ -734,9 +734,9 @@ class auto_or_unknown_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      error (_("type printing not implemented for language \"%s\""),
>  	   natural_name ());
> @@ -1094,6 +1094,25 @@ language_lookup_primitive_type_as_symbol (const struct language_defn *la,
>    return sym;
>  }
>  
> +/* See language.h.  */
> +
> +void
> +language_defn::print_type (struct type *type, const char *varstring,
> +			   struct ui_file *stream, int show, int level,
> +			   const struct type_print_options *flags) const
> +{
> +  /* Print things here which should print the same for every language,
> +     before dispatching to the virtual method.  */
> +  if (type->code () == TYPE_CODE_INTERNAL_FUNCTION)
> +    {
> +      c_print_type (type, varstring, stream, show, level,
> +		    la_language, flags);
> +      return;
> +    }
> +
> +  do_print_type (type, varstring, stream, show, level, flags);
> +}
> +
>  /* Initialize the language routines.  */
>  
>  void _initialize_language ();
> diff --git a/gdb/language.h b/gdb/language.h
> index a51ddf97381..ebe3ffff00e 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -461,11 +461,24 @@ struct language_defn
>    /* Print TYPE to STREAM using syntax appropriate for this language.
>       LEVEL is the depth to indent lines by.  VARSTRING, if not NULL or the
>       empty string, is the name of a variable and TYPE should be printed in
> -     the form of a declaration of a variable named VARSTRING.  */
> +     the form of a declaration of a variable named VARSTRING.
>  
> -  virtual void print_type (struct type *type, const char *varstring,
> -			   struct ui_file *stream, int show, int level,
> -			   const struct type_print_options *flags) const = 0;
> +     This is the public-facing method, which contains code that is
> +     common to all languages, and then dispatches to the virtual
> +     do_print_type method.  */
> +  void print_type (struct type *type, const char *varstring,
> +		   struct ui_file *stream, int show, int level,
> +		   const struct type_print_options *flags) const;
> +
> +protected:
> +
> +  /* Implements actual language-specific parts of print_type.
> +     Arguments and return are like the print_type method.  */
> +  virtual void do_print_type (struct type *type, const char *varstring,
> +			      struct ui_file *stream, int show, int level,
> +			      const struct type_print_options *flags) const = 0;
> +
> +public:
>  
>    /* PC is possibly an unknown languages trampoline.
>       If that PC falls in a trampoline belonging to this language, return
> diff --git a/gdb/m2-lang.h b/gdb/m2-lang.h
> index cda6e241c8c..7d7556fabbd 100644
> --- a/gdb/m2-lang.h
> +++ b/gdb/m2-lang.h
> @@ -73,9 +73,9 @@ class m2_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      m2_print_type (type, varstring, stream, show, level, flags);
>    }
> diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
> index f43d158a770..7964e6da8c5 100644
> --- a/gdb/objc-lang.c
> +++ b/gdb/objc-lang.c
> @@ -273,9 +273,9 @@ class objc_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      c_print_type (type, varstring, stream, show, level, la_language, flags);
>    }
> diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c
> index 3e4a9c360b2..14df2756360 100644
> --- a/gdb/opencl-lang.c
> +++ b/gdb/opencl-lang.c
> @@ -959,9 +959,9 @@ class opencl_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override
>    {
>      /* We nearly always defer to C type printing, except that vector types
>         are considered primitive in OpenCL, and should always be printed
> diff --git a/gdb/p-lang.h b/gdb/p-lang.h
> index 662079114ed..c16a4cd528e 100644
> --- a/gdb/p-lang.h
> +++ b/gdb/p-lang.h
> @@ -88,9 +88,9 @@ class pascal_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override;
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override;
>  
>    /* See language.h.  */
>  
> diff --git a/gdb/p-typeprint.c b/gdb/p-typeprint.c
> index 7458aa6c095..568ff61b607 100644
> --- a/gdb/p-typeprint.c
> +++ b/gdb/p-typeprint.c
> @@ -37,9 +37,9 @@
>  /* See language.h.  */
>  
>  void
> -pascal_language::print_type (struct type *type, const char *varstring,
> -			     struct ui_file *stream, int show, int level,
> -			     const struct type_print_options *flags) const
> +pascal_language::do_print_type (struct type *type, const char *varstring,
> +				struct ui_file *stream, int show, int level,
> +				const struct type_print_options *flags) const
>  {
>    enum type_code code;
>    int demangled_args;
> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
> index fb9db9fe31b..8424736e045 100644
> --- a/gdb/rust-lang.c
> +++ b/gdb/rust-lang.c
> @@ -1626,9 +1626,9 @@ rust_language::language_arch_info (struct gdbarch *gdbarch,
>  /* See language.h.  */
>  
>  void
> -rust_language::print_type (struct type *type, const char *varstring,
> -			   struct ui_file *stream, int show, int level,
> -			   const struct type_print_options *flags) const
> +rust_language::do_print_type (struct type *type, const char *varstring,
> +			      struct ui_file *stream, int show, int level,
> +			      const struct type_print_options *flags) const
>  {
>    print_offset_data podata (flags);
>    rust_internal_print_type (type, varstring, stream, show, level,
> diff --git a/gdb/rust-lang.h b/gdb/rust-lang.h
> index 89e03550fb7..ef95fbd3f3d 100644
> --- a/gdb/rust-lang.h
> +++ b/gdb/rust-lang.h
> @@ -114,9 +114,9 @@ class rust_language : public language_defn
>  
>    /* See language.h.  */
>  
> -  void print_type (struct type *type, const char *varstring,
> -		   struct ui_file *stream, int show, int level,
> -		   const struct type_print_options *flags) const override;
> +  void do_print_type (struct type *type, const char *varstring,
> +		      struct ui_file *stream, int show, int level,
> +		      const struct type_print_options *flags) const override;
>  
>    /* See language.h.  */
>  
>
> base-commit: 141cd158423a5ee248245fb2075fd2e5a580cff2
> -- 
> 2.36.0