Move fix Issue 12153 to frontend

[ibuclaw]

See #3285

Moves the fix into the front-end so everyone benefits.

NB: I haven't yet read any of the comments in that PR.

[ibuclaw]

Responding to comments in the other PR (From what I gather, Kenji did initially fix it in the frontend?)

This pull changes the behavior of the following code:

import core.stdc.stdio;

bool b;
int[3] x;
int[3] y;

void main()
{
    printf("%p\n", x.ptr);
    printf("%p\n", y.ptr);
    printf("%p\n", (b ? x : y)[].ptr);
}

With gdc and this PR, the behaviour in the above test doesn't change.

[ibuclaw]

Likewise this code samples does not change behaviour:

int[3] x;
const int[3] y = x;

Nor this:

import core.stdc.stdio;

bool b;
struct Copy
{
    Copy* slice() { return &this; }
}
Copy x;
Copy func()
{
    return x;
};

void main()
{
    printf("%p\n", (b ? func().slice : func().slice));
    printf("%p\n", (b ? func() : func()).slice);
}

[ibuclaw]

Pinging @yebblies and @9rnsr - as you were involved in this first.

[yebblies]

IIUC this implementing what I suggested in this comment but has no effect on my other concerns from that PR. I'll have to take another look because I can't remember how & and ?: are handled.

[ibuclaw]

IIUC this implementing what I suggested in this comment but has no effect on my other concerns from that PR.

Correct. Here, the transformation is done on the lhs of an AssignExp only.

[ibuclaw]

Have you had any time to mull over this?

[yebblies]

Nope.

[yebblies]

Shouldn't the condition be if (se->e1->type->toBaseType()->ty == Tsarray) instead of if (se->e1->op == TOKquestion)? Are then any cases other than slicing a static array (then assigning to it) that require a modifiable lvalue?

[9rnsr]

This change will introduce a regression with following case:

    int[1] i, j;
    bool b = true;
    int n = 0;
    (b ? i : j)[0..1][n..$] = [4];
    assert(i == [4]);

So the better code would be:

        // For conditional operator, both branches need conversion.
        SliceExp *se = (SliceExp *)e1;
        while (se->e1->op == TOKslice)
            se = (SliceExp *)se->e1;
        if (se->e1->type->toBasetype()->ty == Tsarray)
        {
            se->e1 = se->e1->modifiableLvalue(sc, e1);
            if (se->e1->op == TOKerror)
                return se->e1;
        }

Additionally, I'd keep the code in addressElem to make glue layer robust. Because the translation itself is still valid even if it would be a dead code.

[ibuclaw]

This change will introduce a regression with following case:

Fixed and added regression test.

Additionally, I'd keep the code in addressElem to make glue layer robust. Because the translation itself is still valid even if it would be a dead code.

That's fine, you understand it better than me.

[ibuclaw]

Are then any cases other than slicing a static array (then assigning to it) that require a modifiable lvalue?

Not that I can think off the top of my head.

[ibuclaw]

Shouldn't have blindly copied and pasted your proposed fix @9rnsr :)

[ibuclaw]

Are then any cases other than slicing a static array (then assigning to it) that require a modifiable lvalue?

Not that I can think off the top of my head.

What you have to watch out for are operations that do not have any side-effects, but may still create a temporary. Conditional operators fall under this category.

[ghost]

Are we ok to go forward with this? @9rnsr @yebblies

[yebblies]

Auto-merge toggled on