Re: [PATCH v2 02/11] [PR gdb/14441] gdb: gdbtypes: change {lookup,make}_reference_type() API

[Keith Seitz <keiths@redhat.com>]

On 01/19/2016 10:53 AM, Artemiy Volkov wrote:
> 
> ./Changelog:
> 
> 2016-01-19  Artemiy Volkov  <artemiyv@acm.org>
> 
>         * gdb/dwarf2read.c (read_tag_reference_type): Use
>         lookup_lvalue_reference_type() instead of lookup_reference_type().
>         * gdb/eval.c (evaluate_subexp_standard): Likewise.
>         * gdb/f-exp.y: Likewise.

Nit: Instead of listing a bunch of functions and "Likewise" or "Ditto,"
I think we're using the shorthand: (func1, func1, func3): Use blah blah.
[Sadly does not extend to filenames.]

>         * gdb/gdbtypes.c (make_reference_type): Generalize with rvalue
>         reference types.
>         (lookup_reference_type): Generalize with rvalue reference types.
>         (lookup_lvalue_reference_type): New convenience wrapper for
>         lookup_reference_type().
>         (lookup_rvalue_reference_type): Likewise.
>         * gdb/gdbtypes.h: Change interface for
>         {make,lookup}_{rvalue,}_reference_type().

I would prefer that these function be listed explicitly -- it makes
searching through the changelogs a lot easier, but then I would prefer a
little better description of the change, too. The entry mentions
changing the interface, but it doesn't say how.

>         * gdb/guile/scm-type.c (gdbscm_type_reference): Use
>         lookup_lvalue_reference_type() instead of lookup_reference_type().
>         * gdb/guile/scm-value.c (gdbscm_value_dynamic_type): Likewise.
>         * gdb/parse.c (follow_types): Likewise.
>         * gdb/python/py-type.c (typy_reference): Likewise.
>         (typy_lookup_type): Likewise.
>         * gdb/python/py-value.c (valpy_get_dynamic_type): Likewise.
>         (valpy_getitem): Likewise.
>         * gdb/python/py-xmethods.c (gdbpy_get_xmethod_result_type):
>         Likewise.
>         (gdbpy_invoke_xmethod): Likewise.

Lotsa "likewise"s here! :-)

>         * gdb/stabsread.c: Provide extra argument to make_reference_type()
>         call.
>         * gdb/valops.c (value_ref): Use lookup_lvalue_reference_type()
>         instead of lookup_reference_type().
>         (value_rtti_indirect_type): Likewise.
> ---

> diff --git a/gdb/eval.c b/gdb/eval.c
> index 78ad946..729f473 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -2789,7 +2789,7 @@ evaluate_subexp_standard (struct type *expect_type,
>  
>  	      if (TYPE_CODE (check_typedef (type)) != TYPE_CODE_REF)
>  		{
> -		  type = lookup_reference_type (type);
> +                 type = lookup_lvalue_reference_type (type);

Indentation looks off. Please double-check.

>  		  result = allocate_value (type);
>  		}
>  	    }
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index f129b0e..058b77d 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -382,17 +382,23 @@ lookup_pointer_type (struct type *type)
>  }
>  
>  /* Lookup a C++ `reference' to a type TYPE.  TYPEPTR, if nonzero,
> -   points to a pointer to memory where the reference type should be
> -   stored.  If *TYPEPTR is zero, update it to point to the reference
> -   type we return.  We allocate new memory if needed.  */
> +   points to a pointer to memory where the reference type should be stored.
> +   If *TYPEPTR is zero, update it to point to the reference type we return.
> +   REFCODE denotes the kind of reference type to lookup (lvalue or rvalue
> +   reference). We allocate new memory if needed.  */

There's a lot less here that changed than this diff would indicate. It
is okay to tack on an extra sentence describing the new parameter,
leaving the rest unchanged.

>  struct type *
> -make_reference_type (struct type *type, struct type **typeptr)
> +make_reference_type (struct type *type, struct type **typeptr,
> +                      enum type_code refcode)
>  {
>    struct type *ntype;	/* New type */
> +  struct type **reftype;
>    struct type *chain;
>  
> -  ntype = TYPE_REFERENCE_TYPE (type);
> +  gdb_assert (refcode == TYPE_CODE_REF || refcode == TYPE_CODE_RVALUE_REF);
> +
> +  ntype = (refcode == TYPE_CODE_REF ? TYPE_REFERENCE_TYPE (type)
> +           : TYPE_RVALUE_REFERENCE_TYPE (type));

From comments in patch 1/11, I don't think this is necessary. We know
it's a reference type, and TYPE_REFERENCE_TYPE will get us the
reference's type, either rvalue or lvalue.

>    if (ntype)
>      {
> @@ -421,7 +427,11 @@ make_reference_type (struct type *type, struct type **typeptr)
>      }
>  
>    TYPE_TARGET_TYPE (ntype) = type;
> -  TYPE_REFERENCE_TYPE (type) = ntype;
> +  reftype = (refcode == TYPE_CODE_REF ? &TYPE_REFERENCE_TYPE (type)
> +             : &TYPE_RVALUE_REFERENCE_TYPE (type));
> +
> +  if(!*reftype)
> +    *reftype = ntype;
>  
>    /* FIXME!  Assume the machine has only one representation for
>       references, and that it matches the (only) representation for
> @@ -429,10 +439,9 @@ make_reference_type (struct type *type, struct type **typeptr)
>  
>    TYPE_LENGTH (ntype) =
>      gdbarch_ptr_bit (get_type_arch (type)) / TARGET_CHAR_BIT;
> -  TYPE_CODE (ntype) = TYPE_CODE_REF;
> +  TYPE_CODE (ntype) = refcode;
>  
> -  if (!TYPE_REFERENCE_TYPE (type))	/* Remember it, if don't have one.  */
> -    TYPE_REFERENCE_TYPE (type) = ntype;
> +  *reftype = ntype;
>  
>    /* Update the length of all the other variants of this type.  */
>    chain = TYPE_CHAIN (ntype);

This whole hunk is probably simplified or (largely) unnecessary if we've
agreed to remove struct type.rvalue_reference_type (from patch 1/11).

> @@ -449,9 +458,23 @@ make_reference_type (struct type *type, struct type **typeptr)
>     details.  */
>  
>  struct type *
> -lookup_reference_type (struct type *type)
> +lookup_reference_type (struct type *type, enum type_code refcode)
> +{
> +  return make_reference_type (type, (struct type **) 0, refcode);
> +}
> +
> +/* Separate convenience functions for lvalue and rvalue references. */
> +
> +struct type *
> +lookup_lvalue_reference_type (struct type *type)
> +{
> +  return lookup_reference_type (type, TYPE_CODE_REF);
> +}
> +
> +struct type *
> +lookup_rvalue_reference_type (struct type *type)
>  {
> -  return make_reference_type (type, (struct type **) 0);
> +  return lookup_reference_type (type, TYPE_CODE_RVALUE_REF);
>  }
>  

Our coding standard requires a comment before each function, even
trivial ones. [Please don't shoot the messenger!] Additionally, if these
are exported, the comment explaining the purpose of the function should
be put in the header and "See description in foo.h." (or similar) should
be sufficient for the comment ahead of the implementation.

>  /* Lookup a function type that returns type TYPE.  TYPEPTR, if
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 52419b4..d9b6b9e 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1725,9 +1725,13 @@ extern void append_flags_type_flag (struct type *type, int bitpos, char *name);
>  extern void make_vector_type (struct type *array_type);
>  extern struct type *init_vector_type (struct type *elt_type, int n);
>  
> -extern struct type *lookup_reference_type (struct type *);
> +extern struct type *lookup_reference_type (struct type *, enum type_code);
> +extern struct type *lookup_lvalue_reference_type (struct type *);
> +extern struct type *lookup_rvalue_reference_type (struct type *);
>  
> -extern struct type *make_reference_type (struct type *, struct type **);
> +
> +extern struct type *make_reference_type (struct type *, struct type **,
> +                                         enum type_code);
>  
>  extern struct type *make_cv_type (int, int, struct type *, struct type **);
>  

See previous comment.

> diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
> index 1cdf953..1681a77 100644
> --- a/gdb/guile/scm-value.c
> +++ b/gdb/guile/scm-value.c
> @@ -601,7 +601,7 @@ gdbscm_value_dynamic_type (SCM self)
>  	      if (was_pointer)
>  		type = lookup_pointer_type (type);
>  	      else
> -		type = lookup_reference_type (type);
> +               type = lookup_lvalue_reference_type (type);

Indentation?
>  	    }
>  	}
>        else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index 03cc8d9..385cb53 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -827,7 +827,7 @@ typy_lookup_type (struct demangle_component *demangled,
>  	  switch (demangled_type)
>  	    {
>  	    case DEMANGLE_COMPONENT_REFERENCE:
> -	      rtype =  lookup_reference_type (type);
> +             rtype = lookup_lvalue_reference_type (type);
>  	      break;

Indentation?

>  	    case DEMANGLE_COMPONENT_POINTER:
>  	      rtype = lookup_pointer_type (type);
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index 7dba0ad..9d479ea 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -376,7 +376,7 @@ valpy_get_dynamic_type (PyObject *self, void *closure)
>  	      if (was_pointer)
>  		type = lookup_pointer_type (type);
>  	      else
> -		type = lookup_reference_type (type);
> +               type = lookup_lvalue_reference_type (type);
>  	    }
>  	}

Indentation?

>        else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> @@ -766,7 +766,7 @@ valpy_getitem (PyObject *self, PyObject *key)
>  	  if (TYPE_CODE (val_type) == TYPE_CODE_PTR)
>  	    res_val = value_cast (lookup_pointer_type (base_class_type), tmp);
>  	  else if (TYPE_CODE (val_type) == TYPE_CODE_REF)
> -	    res_val = value_cast (lookup_reference_type (base_class_type), tmp);
> +           res_val = value_cast (lookup_lvalue_reference_type (base_class_type), tmp);
>  	  else
>  	    res_val = value_cast (base_class_type, tmp);
>  	}

Indentation? The line is also too long. See
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits
.

> diff --git a/gdb/stabsread.c b/gdb/stabsread.c
> index 74260b7..8a50dbd 100644
> --- a/gdb/stabsread.c
> +++ b/gdb/stabsread.c
> @@ -1772,7 +1772,7 @@ again:
>  
>      case '&':                  /* Reference to another type */
>        type1 = read_type (pp, objfile);
> -      type = make_reference_type (type1, dbx_lookup_type (typenums, objfile));
> +      type = make_reference_type (type1, dbx_lookup_type (typenums, objfile), TYPE_CODE_REF);
>        break;
>  
>      case 'f':                  /* Function returning another type */

The changed line is too long.

Keith