Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Jason Merrill <jason at redhat dot com>]

On Fri, Apr 22, 2016 at 2:12 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Apr 22, 2016 at 09:43:31AM -0400, Jason Merrill wrote:
>> On 04/22/2016 07:50 AM, Marek Polacek wrote:
>> >This PR shows that we generate wrong code with the x ?: y extension in case the
>> >first operand contains either predecrement or preincrement.  The problem is
>> >that we don't emit SAVE_EXPR, thus the operand is evaluated twice, which it
>> >should not be.
>> >
>> >While ++i or --i can be lvalues in C++, i++ or i-- can not.  The code in
>> >build_conditional_expr_1 has:
>> >  4635       /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  */
>> >  4636       if (real_lvalue_p (arg1))
>> >  4637         arg2 = arg1 = stabilize_reference (arg1);
>> >  4638       else
>> >  4639         arg2 = arg1 = save_expr (arg1);
>> >so for ++i/--i we call stabilize_reference, but that doesn't know anything
>> >about PREINCREMENT_EXPR or PREDECREMENT_EXPR and just returns the same
>> >expression, so SAVE_EXPR isn't created.
>> >
>> >I think one fix would be to teach stabilize_reference what to do with those,
>> >similarly to how we handle COMPOUND_EXPR there.
>>
>> Your change will turn the expression into an rvalue, so that isn't enough.
>
> Oops.
>
>> We need to turn the expression into some sort of _REF before passing it to
>> stabilize_reference, perhaps by factoring out the cast-to-reference code
>> from force_paren_expr.  This should probably be part of a
>> cp_stabilize_reference function that replaces all uses of
>> stabilize_reference in the front end.
>
> Thanks, this magic seems to work.  So something like the following?  I didn't
> put the cast-to-reference code into its separate function because it didn't
> seem convenient, but I can work on that, too, if you prefer.

> +cp_stabilize_reference (tree ref)
> +{
> +  if (TREE_CODE (ref) == PREINCREMENT_EXPR
> +      || TREE_CODE (ref) == PREDECREMENT_EXPR)

I think we want to do this for anything stabilize_reference doesn't
handle specifically, not just pre..crement.

Jason