Re: [PATCH v2 09/11] [PR gdb/14441] gdb: gdbtypes: add rvalue references to overloading resolution

[Keith Seitz <keiths@redhat.com>]

On 01/19/2016 10:53 AM, Artemiy Volkov wrote:
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index fd17d3c..4bb98c9 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -3464,6 +3466,20 @@ rank_one_type (struct type *parm, struct type *arg, struct value *value)
>  {
>    struct rank rank = {0,0};
>  
> +  /* Disallow an rvalue argument to bind to a non-const lvalue reference
> +     parameter and an lvalue argument to bind to an rvalue reference
> +     parameter. */

Nit: two spaces after all sentences. ["parameter.  /*"]

> +
> +  if ((value != NULL)
> +      &&
> +      ((TYPE_CODE (parm) == TYPE_CODE_REF
> +       && !TYPE_CONST (parm->main_type->target_type)
> +       && VALUE_LVAL (value) == not_lval)
> +      ||
> +       (TYPE_CODE (parm) == TYPE_CODE_RVALUE_REF
> +       && VALUE_LVAL (value) != not_lval)))
> +    return INCOMPATIBLE_TYPE_BADNESS;
> +

I would probably split this up into two statements instead of using "||"
to combine, leading each statement with the appropriate comment/citation
from the spec.

if (value != NULL)
  {
    /* An rvalue argument cannot be bound to a non-const lvalue
       reference parameter...  */
    if (VALUE_LVAL (value) == not_lval
        && TYPE_CODE (parm) == TYPE_CODE_REF
        && !TYPE_CONST (TYPE_TARGET_TYPE (parm)))
      return INCOMPATIBLE_TYPE_BADNESS;

    /* ... and an lvalue argument cannot be bound to an rvalue
       reference parameter.  [C++ 13.3.3.1.4p3]  */
   if (VALUE_LVAL (value) != not_lval
       && TYPE_CODE (parm) == TYPE_CODE_RVALUE_REF)
     return INCOMPATIBLE_TYPE_BADNESS;
  }

This is just so much easier to read IMO.

For the first test here, is using the target type of the reference
sufficient? Do we need to resolve typedefs? In many places, I see the
code looping over references (and pointers) to get to the underlying type:

  for (;;)
    {
      type = check_typedef (type);
      if (TYPE_CODE (type) != TYPE_CODE_PTR
          && TYPE_CODE (type) != TYPE_CODE_REF)
        break;
      type = TYPE_TARGET_TYPE (type);
    }

Maybe you have a test that covers this already (where a reference refers
to a typedef'd type)? [If not, please add.]

>    if (types_equal (parm, arg))
>      return EXACT_MATCH_BADNESS;
>  
> @@ -3473,6 +3489,36 @@ rank_one_type (struct type *parm, struct type *arg, struct value *value)
>    if (TYPE_CODE (arg) == TYPE_CODE_TYPEDEF)
>      arg = check_typedef (arg);
>  
> +  /* An lvalue reference to a function should get higher priority than an
> +     rvalue reference to a function. */
> +
> +  if (value != NULL && TYPE_CODE (arg) == TYPE_CODE_RVALUE_REF
> +      && TYPE_CODE (TYPE_TARGET_TYPE (arg)) == TYPE_CODE_FUNC)
> +    return (sum_ranks (rank_one_type (parm,
> +            lookup_pointer_type (TYPE_TARGET_TYPE (arg)), NULL),
> +            DIFFERENT_REFERENCE_TYPE_BADNESS));

Multi-line statements should be encapsulated in a block. [I know a *lot*
of patches have been committed where submitters (and maintainers)
haven't followed the rule, but it *is* in our internal coding style
wiki.
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Whitespaces
]

This appears several times.

Keith