Re: [C++] [PR84231] overload on cond_expr in template

[Jason Merrill <jason at redhat dot com>]

On Fri, Mar 2, 2018 at 2:57 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 28, 2018, Jason Merrill <jason@redhat.com> wrote:
>
>> On Wed, Feb 28, 2018 at 12:24 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> +  if (processing_template_decl)
>>> +    result_type = cp_build_reference_type (result_type, !is_lvalue);
>
>> If !is_lvalue, we don't want a reference type at all, as the result is
>> a prvalue.  is_lvalue should probably rename to is_glvalue.
>
> I ended up moving the above to the path in which we deal with lvalues
> and xvalues.  I still renamed the variable as suggested, though I no
> longer use it.
>
>> The second argument to cp_build_reference_type should be xvalue_p (arg2).
>
> I thought of adding a comment as to why testing just arg2 was correct,
> but then moving the code kind of made it obvious, didn't it?
>
>>> +      /* Except for type-dependent exprs, a REFERENCE_TYPE will
>>> +        indicate whether its result is an lvalue or so.
>
>> "or not" ?
>
> I meant "or so" as in "or other kinds of reference values".
>
>>> +      if (processing_template_decl
>>> +         && !type_dependent_expression_p (CONST_CAST_TREE (ref)))
>>> +       return clk_none;
>
>> I think we want to return clk_class for a COND_EXPR of class type.
>
> or array, like the default case, I suppose.
>
>> Can we actually get here for a type-dependent expression?  I'd think
>> we'd never get as far as asking whether a type-dependent expression is
>> an lvalue, since in general we can't answer that question.
>
> We shouldn't, indeed.  And AFAICT we don't; I've added an assert to make
> sure.
>
> [C++] [PR84231] overload on cond_expr in template
>
> A non-type-dependent COND_EXPR within a template is reconstructed with
> the original operands, after one with non-dependent proxies is built to
> determine its result type.  This is problematic because the operands of
> a COND_EXPR determined to be an rvalue may have been converted to denote
> their rvalue nature.  The reconstructed one, however, won't have such
> conversions, so lvalue_kind may not recognize it as an rvalue, which may
> lead to e.g. incorrect overload resolution decisions.
>
> If we mistake such a COND_EXPR for an lvalue, overload resolution might
> regard a conversion sequence that binds it to a non-const reference as
> viable, and then select that over one that binds it to a const
> reference.  Only after template substitution would we rebuild the
> COND_EXPR, realize it is an rvalue, and conclude the reference binding
> is ill-formed, but at that point we'd have long discarded any alternate
> candidates we could have used.
>
> This patch modifies the logic that determines whether a
> (non-type-dependent) COND_EXPR in a template is an lvalue, to rely on
> its type, more specifically, on the presence of a REFERENCE_TYPE
> wrapper.  In order to avoid a type bootstrapping problem, the
> REFERENCE_TYPE that wraps the type of some such COND_EXPRs is
> introduced earlier, so that we don't have to test for lvalueness of
> the expression using the very code that we wish to change.
>
>
> for  gcc/cp/ChangeLog
>
>         PR c++/84231
>         * tree.c (lvalue_kind): Use presence/absence of REFERENCE_TYPE
>         only while processing template decls.
>         * typeck.c (build_x_conditional_expr): Move wrapping of
>         reference type around type...
>         * call.c (build_conditional_expr_1): ... here.  Rename
>         is_lvalue to is_glvalue.
>         * parser.c (cp_parser_fold_expression): Catch REFERENCE_REF_P
>         INDIRECT_REF of COND_EXPR too.
>
> for  gcc/testsuite/ChangeLog
>
>         PR c++/84231
>         * g++.dg/pr84231.C: New.
> ---
>  gcc/cp/call.c                  |   11 +++++++----
>  gcc/cp/parser.c                |    4 +++-
>  gcc/cp/tree.c                  |   15 +++++++++++++++
>  gcc/cp/typeck.c                |    4 ----
>  gcc/testsuite/g++.dg/pr84231.C |   29 +++++++++++++++++++++++++++++
>  5 files changed, 54 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/pr84231.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 11fe28292fb1..6707aa2d3f02 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -4782,7 +4782,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
>    tree arg3_type;
>    tree result = NULL_TREE;
>    tree result_type = NULL_TREE;
> -  bool is_lvalue = true;
> +  bool is_glvalue = true;
>    struct z_candidate *candidates = 0;
>    struct z_candidate *cand;
>    void *p;
> @@ -5037,7 +5037,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
>           return error_mark_node;
>         }
>
> -      is_lvalue = false;
> +      is_glvalue = false;
>        goto valid_operands;
>      }
>    /* [expr.cond]
> @@ -5155,6 +5155,9 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree arg2, tree arg3,
>        && same_type_p (arg2_type, arg3_type))
>      {
>        result_type = arg2_type;
> +      if (processing_template_decl)
> +       result_type = cp_build_reference_type (result_type, xvalue_p (arg2));

Let's add a comment along the lines of

/* Let lvalue_kind know this was a glvalue.  */

OK with that change.

Jason