argprefix: fix evaluation order and missing cleanup bug [bugzilla 14903]#5151
argprefix: fix evaluation order and missing cleanup bug [bugzilla 14903]#5151MartinNowak merged 2 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
argprefixD
branch
2 times, most recently
from
e5b23d1 to
fb5797f
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. Note that previously, `appendToPrefix` would be set by the first arg with dtor (if a subsequent arg may throw), and only reset when processing the last throwing arg (i == lastthrow). So all non-ref/out/lazy args inbetween would be added to `eprefix`, leading to this staggering evaluation order: [firstDtor..lastThrow (except for ref/out/lazy)] [0..firstDtor] [firstDtor..lastThrow (only ref/out/lazy)] [lastThrow..$] Also prepare for potential right-to-left arguments evaluation order (array ops).
|
Am I really the only one deeming this important enough to be included in 2.069? |
|
I agree, this should have really gone into 2.069. It might be too late now, though, since it's not really a regression fix. (ping @MartinNowak) |
|
Sorry, we won't merge something risky into stable, and just looking at the diff is pretty hard. |
|
So another release will pass by... just let me express that waiting for a pull fixing 2 rather horrible issues (missing dtor calls + totally unintuitive argument evaluation order with potentially nasty side fx) to be merged is pretty annoying. The first pull was for C++ dmd back in August, i.e., 5 months ago; it's been green since then, so it cannot be that wrong - and by no means more wrong than the current implementation ;). If I wasn't part of the community already (to some limited extent), I'd be off for good; if it had been like this with LDC too, I would have never gotten involved. No offense to anyone (especially @MartinNowak), but we should try not to scare off new people with a mindset similar to mine, by finding a better prioritization system. |
|
@WalterBright @MartinNowak @yebblies now with 2.070 out the door what do you guys think about discussing this? |
|
You're absolutely right @kinke, will try to review this within the next few days.
We can't currently keep up with github, but if you want to bring up something important, please participate in our spring planings or simply write a mail. Are you aware of Issue 14708 – destructor for temporary not called during stack unwinding and #5110 (comment). |
|
Thanks for the follow-up. 👍 I'm at work now so I can't dig into the 2 mentioned issues. But they may very likely be fixed by this. As I said, one thing is the args evaluation order. The other one is a bug wrt. to premature disabling of the temporaries' dtors in case the right-most potentially throwing arg does throw. Disabling is only okay if the callee gets actually called, as it will destruct the args. The previous code disabled all dtors right before evaluating the last potentially throwing arg though (edit: if it yielded a struct, see 1st commit msg), so no dtors were called if it did throw. That's a short summary to give you the required context. ;)
We had to extend LDC for this to work; it keeps a stack of live temporaries to be destructed for the current statement, and takes care of destructing them on a throw. This is the other part of issue 14903, and isn't tackled by this PR. The TODO parts in the tests file are related to this aspect... |
That would be a good solution, and one that could mostly be implemented in the frontend. |
|
Auto-merge toggled on |
|
Thank you very much! |
argprefix: fix evaluation order and missing cleanup bug [bugzilla 14903]
4 participants
DDMD port of #4885.