Fix argprefix: destruct temporary args if last throwing arg throws#4885
Fix argprefix: destruct temporary args if last throwing arg throws#4885kinke wants to merge 3 commits intodlang:masterfrom
Conversation
Previously, if the last potentially throwing argument expression's `type->baseElemOf()` yielded a struct type, the arg would incorrectly NOT be added to `eprefix`. This led to the destructor gate being set to true (disabling all dtors) right BEFORE evluating the last potentially throwing argument. So in case it threw, no dtors would be triggered.
kinke
force-pushed
the
argprefix
branch
2 times, most recently
from
da15ebd to
98e1205
Compare
|
Needs a test case :) |
Reduce code duplication, simplify and clarify some points. The only functional changes are additional asserts and the name of temporaries with no dtor in the middle of `eprefix` now beginning with `pfy` instead of `pfx`. Note that previously, `appendToPrefix` would be set by the first arg with dtor (if there may be subsequent throws), and only reset when processing the last throwing arg (i == lastthrow). So all non-reference args inbetween are added to `eprefix`, even args without dtor before the first throwing arg (i < firstthrow).
|
Of course, but there's still stuff to be fixed here. The evaluation order seems horribly broken to me. Consider this: void foo(int a, int b, StructWithDtor c, int d, int e, ref int f, int g, int h);
foo(1, 2, StructWithDtor(), 4, mayThrow(), getIntRef(), mayThrow(), 8):
firstthrow = e, lastthrow = g
left-to-right: [prefix] c, d, e, g [/prefix], a, b, f, h
right-to-left: [prefix] c, d, e, g [/prefix], h, f, b, aWouldn't it just make much more sense to put all args into |
|
Arrayops is a sensitive topic that probably is best to leave out of this PR. :) |
kinke
force-pushed
the
argprefix
branch
2 times, most recently
from
ee4e8a5 to
3441f57
Compare
* If argprefix is required, make sure it covers the very first argument up to and including the last potentially throwing argument. Only evaluate subsequent arguments directly. This also includes ref and out params, but not lazy ones. * Prepare for right-to-left arguments evaluation order (array ops).
|
Now includes test cases and fixes the evaluation order too. Doesn't make any assumptions about right-to-left order yet, but is prepared to handle that. An open question mark is how to deal with lazy args in argprefix. Currently, they are excluded from argprefix (as in upstream). |
|
LGTM, but ideally @9rnsr would also have a quick look at this. |
Everything LTR!
Lazy args should be skipped. |
Well, at least in LDC we explicitly evaluate the arrayops args from right-to-left: https://github.com/ldc-developers/ldc/blob/merge-2.067/gen/tocall.cpp#L403. Is that incorrect by now? |
We've reached an impasse between us and Walter. #4035 I think the only way we are going to move forward is to a fork between arrayops on x86 vs. every other platform. |
|
Superseded by a DDMD port #5151. |
3 participants
Fixes one aspect of https://issues.dlang.org/show_bug.cgi?id=14903.
Doesn't tackle the exception chaining aspect regarding throwing destructors of temporaries, that's back-end stuff.