Fix evaluation order issues

[JohanEngelen]

Resolves #1617 .

There is a lot of refactoring potential here (duplication of code), but it is additional work and we first need to get this right.

[JohanEngelen]

Let's see what the CI testers say... meanwhile, I am going to try and compile Weka's codebase with it.

Edit: so there was a bug due to copy-paste error (casts that are needed for bitwise ops should not be there for the other ops), fixed now.
Weka's codebase successfully compiles with this change and all their checks pass.

[JohanEngelen]

There is some funny business happening in the code that is wholly unnecessary I think. Let's see what the CI testers think... First I'll commit the (I hope) fully fixed version, then a second commit with the funny business cut out.

Edit: OK, without it, things break, so I'll keep it for now.

[JohanEngelen]

Cancelled all CI tests, amend+pushforce to start all of them again.
Hopefully mergable when all is green.

[kinke]

Yes I think it's needed. Test:

int x = 6;
int inc() { ++x; return 4; }
(x++) <<= inc();
assert(x == 8 << 4);

[Note that I haven't actually tested this code. ;)]
Using the lhs_val for the lhs makes sure we use the lvalue subexpression x which should be 8 by the time we load the lhs for the binop. lhs_fullEvaluation, i.e., (x++), would be 6.

But I now think that this should probably be the case for all other binassign expressions, not just for shifts...

[kinke]

There is some funny business happening in the code that is wholly unnecessary I think.

Hmm? The pre-evaluation of the lvalue subexpression or what do you mean?

Hopefully mergable when all is green.

No rush mate ;) - let's refactor this a bit, 170 additional lines shouldn't be necessary. That doesn't have to be you, I could do that as well, but that might take some time.

[JohanEngelen]

Indeed I thought pre-evaluation of findLvalueExp was not needed (because right after, we evaluate e->e1). But perhaps that's because I don't fully understand what is going on. With your explanation for the <<= case, I think I understand better now why it is needed.

lhs_fullEvaluation is not a good name, do you have a better idea for a name?

(keep pasting testcase ideas too!)

[kinke]

Indeed I thought pre-evaluation of findLvalueExp was not needed (because right after, we evaluate e->e1).

Yep, this isn't a very elegant solution at the moment. The thing is that we need the address to store the result of the binop to, and that the full lhs expression e->e1 may not yield the lvalue directly (e.g., x++). So we currently visit the lhs AST to find the lvalue subexpression (a variable in trivial cases), pre-evaluate that one to get hold of its address, and only then evaluate the whole lhs (the lvalue subexpression's value is cached, so that it isn't evaluated again).
If the lvalue subexpression was always a VarExp, it'd be way easier, and we could simply fetch its address after evaluating the binop, as toElem()enting would have no side effects. But for instance this doesn't work if the lvalue is a reference returned by a function: ref T getLValue() and a binassign expression (getLValue()++) *= 3.

lhs_fullEvaluation is not a good name, do you have a better idea for a name?

lhs_fullEvaluation is the value for the binop's lhs. lhs_val is the lvalue nested in the lhs expression only required as result storage. So I'd probably name these lhsVal and dstVal or so.

Edit: Oh I forgot that the lvalue should probably be the actual lhs for the binop, for all binassign expressions and not just shifts. This means that we only need to fully evaluate e->e1 for the sake of side effects. In this case I wouldn't give lhs_fullEvaluation a name, all we need is the lhsLVal and a toElem(e->e1), the latter with an appropriate comment.

[kinke]

Hmm this may all be a tedious fight with a symptom. Firstly, the lhs x++ is refused by the front-end as it yields no lvalue, makes sense. So I think this whole pre-evaluation thing may only be needed because we don't/used not to always return a DLValue for toElem(e->e1)...

I'll have a go at some refactoring and will then try to get rid of the pre-evaluation.

[kinke]

Continued here: #1623

[JohanEngelen]

@kinke shall I close this one?