Re: [PATCH] c++/8218: Destructors w/arguments.

[Pedro Alves <palves@redhat.com>]

Hi Keith,

In principle, I like the change, as it's obviously a desirable
behavior change, but I have a few concerns.  See below.

> gdb/ChangeLog
> 
> 	PR c++/8218
> 	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> 	at index 0, ignoring STATCIP.
> 	Skip artificial arguments.
> 
> gdb/testsuite/ChangeLog
> 
> 	PR c++/8218
> 	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> 	at index 0, ignoring STATCIP.
> 	Skip artificial arguments.

Wrong entry for testsuite.

> ---
>  gdb/ChangeLog                      |  7 +++++++
>  gdb/c-typeprint.c                  | 11 ++++++++---
>  gdb/testsuite/ChangeLog            |  8 ++++++++
>  gdb/testsuite/gdb.cp/templates.exp | 24 +++++++++++++++---------
>  4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 3ac5170..66cdfbd 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2017-03-08  Keith Seitz  <keiths@redhat.com>
> +
> +	PR c++/8218
> +	* c-typeprint.c (cp_type_print_method_args): Start printing arguments
> +	at index 0, ignoring STATCIP.
> +	Skip artificial arguments.
> +
>  2017-02-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* dwarf2read.c (dwarf2_record_block_ranges): Add forgotten BASEADDR.
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 75f6d61..e504618 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -226,13 +226,18 @@ cp_type_print_method_args (struct type *mtype, const char *prefix,
>  			   language_cplus, DMGL_ANSI);
>    fputs_filtered ("(", stream);
>  
> -  /* Skip the class variable.  */
> -  i = staticp ? 0 : 1;
> +  i = 0;
>    if (nargs > i)
>      {
>        while (i < nargs)
>  	{
> -	  c_print_type (args[i++].type, "", stream, 0, 0, flags);
> +	  struct field arg = args[i++];

The manipulation of 'i' looks a bit obscure.  This is crying
for a "for", IMO:

  if (nargs > 0)
    {
      for (int i = 0; i < nargs; i++)
	{
	  struct field arg = args[i];


> +
> +	  /* Skip any artificial arguments.  */
> +	  if (FIELD_ARTIFICIAL (arg))
> +	    continue;

Can we trust that FIELD_ARTIFICIAL is always set on the implicit this
argument on all debug formats?  I.e., does it work with stabs?

Also, even for DWARF, does it work with debug info produced
by older GCCs?  And older dwarf revisions?  -gdwarf-2, etc.
Can you poke a bit please?  I wouldn't be surprised if we still
needed to treat the first argument specially.

Can you comment on the treatment c_type_print_args gives to
artificial arguments that Tom pointed out in the PR?

Also that function's intro comment talks about C++ "this".
Is that stale?

Thanks,
Pedro Alves