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

[Keith Seitz <keiths@redhat.com>]

On 03/04/2016 07:19 PM, Artemiy Volkov wrote:
> Parameterize lookup_reference_type() and make_reference_type() by the kind of
> reference type we want to look up. Create two wrapper functions
> lookup_{lvalue,rvalue}_reference_type() for lookup_reference_type() to simplify
> the API. Change all callers to use the new API.

Just an FYI: When the time comes to commit this, GIT will require a
single-line description/summary of the patch, followed by a blank line.

> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index f129b0e..a99e878 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -384,15 +384,21 @@ 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.  */
> +   type we return.  We allocate new memory if needed. REFCODE denotes

Need two spaces after "if needed."

> +   the kind of reference type to lookup (lvalue or rvalue reference).  */
>  
>  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));
>  
>    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 != NULL)
> +    *reftype = ntype;
>  
>    /* FIXME!  Assume the machine has only one representation for
>       references, and that it matches the (only) representation for

I would prefer to do:

  if (refcode == TYPE_CODE_REF)
    TYPE_REFERENCE_TYPE (type) = ntype;
  else
    TYPE_RVALUE_REFERENCE_TYPE (type) = ntype;

It's cleaner/smaller/easier to read (at least for me). [Otherwise, a
space is missing between "if" and "(".] See next comment, though.


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

This is slightly different from the original, which only set the new
type if it was unset in the type. Your revised version will
unconditionally do it every time. I have no idea if it makes a
difference or not. Do you?

>    /* Update the length of all the other variants of this type.  */
>    chain = TYPE_CHAIN (ntype);

Keith