Re: [PATCH v2 08/11] [PR gdb/14441] gdb: convert lvalue reference type check to general reference type check

[Keith Seitz <keiths@redhat.com>]

On 01/19/2016 10:53 AM, Artemiy Volkov wrote:
> In almost all contexts (except for overload resolution rules and expression
> semantics), lvalue and rvalue references are equivalent. That means that in
> all but these cases we can replace a TYPE_CODE_REF check to TYPE_REFERENCE
> check. This patch does exactly that.
> 

This is largely a mechanical change, but I do have a few nits to point
out. See below.

May I also bring to your attention that any subsequent patch submissions
keep purely mechanical substitutions (like the majority of this patch)
separate from anything more substantial. This patch contains some
non-mechanical changes, such as cleanups (valops.c:value_cast) and
additional handling for TYPE_CODE_RVALUE_REF in a handful of places.

As a reviewer (and it may just be that I'm not very versed at it), I
find it easy to lose these in a sea of mechanical changes.

> 2016-01-19  Artemiy Volkov  <artemiyv@acm.org>
> 
>         * gdb/aarch64-tdep.c (aarch64_type_align): Change TYPE_CODE_REF
>         check to TYPE_REFERENCE check.
>         (aarch64_extract_return_value): Likewise.
>         (aarch64_store_return_value): Likewise.
>         * gdb/amd64-tdep.c (amd64_classify): Likewise.
[big snip]

We are now encouraging/permitting developers to use the phrase "Update
all callers." or "All callers updated." with large, mechanical changes
like this.

> diff --git a/gdb/eval.c b/gdb/eval.c
> index 729f473..443a08c 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -2509,7 +2509,7 @@ evaluate_subexp_standard (struct type *expect_type,
>  	{
>  	  type = check_typedef (value_type (arg1));
>  	  if (TYPE_CODE (type) == TYPE_CODE_PTR
> -	      || TYPE_CODE (type) == TYPE_CODE_REF
> +	      || TYPE_REFERENCE (type)
>  	  /* In C you can dereference an array to get the 1st elt.  */
>  	      || TYPE_CODE (type) == TYPE_CODE_ARRAY
>  	    )

Superfluous whitespace change?

> @@ -2787,9 +2787,9 @@ evaluate_subexp_standard (struct type *expect_type,
>  	    {
>  	      struct type *type = value_type (result);
>  
> -	      if (TYPE_CODE (check_typedef (type)) != TYPE_CODE_REF)
> +	      if (!TYPE_REFERENCE (type))
>  		{
> -                 type = lookup_lvalue_reference_type (type);
> +		  type = lookup_lvalue_reference_type (type);
>  		  result = allocate_value (type);
>  		}
>  	    }

Same here?

> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 058b77d..fd17d3c 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -3475,10 +3475,11 @@ rank_one_type (struct type *parm, struct type *arg, struct value *value)
>  
>    /* See through references, since we can almost make non-references
>       references.  */
> -  if (TYPE_CODE (arg) == TYPE_CODE_REF)
> +

Superfluous whitespace.

> +  if (TYPE_REFERENCE (arg))
>      return (sum_ranks (rank_one_type (parm, TYPE_TARGET_TYPE (arg), NULL),
>                         REFERENCE_CONVERSION_BADNESS));
> -  if (TYPE_CODE (parm) == TYPE_CODE_REF)
> +  if (TYPE_REFERENCE (parm))
>      return (sum_ranks (rank_one_type (TYPE_TARGET_TYPE (parm), arg, NULL),
>                         REFERENCE_CONVERSION_BADNESS));
>    if (overload_debug)
> diff --git a/gdb/valops.c b/gdb/valops.c
> index cc01689..b4f9ba5 100644
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -360,24 +360,20 @@ value_cast (struct type *type, struct value *arg2)
>    if (value_type (arg2) == type)
>      return arg2;
>  
> -  code1 = TYPE_CODE (check_typedef (type));
> -
>    /* Check if we are casting struct reference to struct reference.  */
> -  if (code1 == TYPE_CODE_REF)
> +  if (TYPE_REFERENCE (check_typedef (type)))
>      {
>        /* We dereference type; then we recurse and finally
>           we generate value of the given reference.  Nothing wrong with 
>  	 that.  */
>        struct type *t1 = check_typedef (type);
>        struct type *dereftype = check_typedef (TYPE_TARGET_TYPE (t1));
> -      struct value *val =  value_cast (dereftype, arg2);
> +      struct value *val = value_cast (dereftype, arg2);
>  
>        return value_ref (val, TYPE_CODE (t1));
>      }
>  
> -  code2 = TYPE_CODE (check_typedef (value_type (arg2)));
> -
> -  if (code2 == TYPE_CODE_REF)
> +  if (TYPE_REFERENCE (check_typedef (value_type (arg2))))
>      /* We deref the value and then do the cast.  */
>      return value_cast (type, coerce_ref (arg2)); 
>  

A bunch of cleanups like this would normally be considered a separate
change. I'm not going to ask anything, but be aware that the final
reviewer may call this out.

> @@ -1729,14 +1725,12 @@ typecmp (int staticp, int varargs, int nargs,
>  	 char *>, and properly access map["hello"], because the
>  	 argument to [] will be a reference to a pointer to a char,
>  	 and the argument will be a pointer to a char.  */
> -      while (TYPE_CODE(tt1) == TYPE_CODE_REF
> -	     || TYPE_CODE (tt1) == TYPE_CODE_PTR)
> -	{
> +      while (TYPE_REFERENCE(tt1) || TYPE_CODE (tt1) == TYPE_CODE_PTR) {
>  	  tt1 = check_typedef( TYPE_TARGET_TYPE(tt1) );
>  	}
>        while (TYPE_CODE(tt2) == TYPE_CODE_ARRAY
>  	     || TYPE_CODE(tt2) == TYPE_CODE_PTR
> -	     || TYPE_CODE(tt2) == TYPE_CODE_REF)
> +	     || TYPE_REFERENCE(tt2))
>  	{
>  	  tt2 = check_typedef (TYPE_TARGET_TYPE(tt2));
>  	}

Formatting errors. I realize you probably just did a text replace, but
since you've touched a handful of the places where there were errors,
please fix those instances. "TYPE_REFERENCE (". You do not need to fix
the other formatting errors here. Just the two lines you changed.

Keith