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

  This patch introduces changes to rank_one_type() dealing with ranking an rvalue
reference type when selecting a best viable function from a set of candidate
functions. The 3 new added rules for rvalue references are:

1) An rvalue argument cannot be bound to a non-const lvalue reference parameter
and an lvalue argument cannot be bound to an rvalue reference parameter.
[C++11 13.3.3.1.4p3]

2) If a conversion to one type of reference is an identity conversion, and a
conversion to the second type of reference is a non-identity conversion, choose
the first type. [C++11 13.3.3.2p3]

3) An rvalue should be first tried to bind to an rvalue reference, and then to
an lvalue reference. [C++11 13.3.3.2p3]

4) An lvalue reference to a function gets higher priority than an rvalue
reference to a function. [C++11 13.3.3.2p3]

./ChangeLog:

2016-01-19  Artemiy Volkov  <artemiyv@acm.org>

        * gdb/gdbtypes.c (rank_one_type): Implement overloading
        resolution rules regarding rvalue references.
---
 gdb/gdbtypes.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
  

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