Re: "finally" handling refactoring (Bug #72213)

Hi, On 05/23/2016 07:24 PM, Nikita Popov wrote:

On Mon, May 23, 2016 at 1:25 PM, Dmitry Stogov <dmitry@zend.com <mailto:dmitry@zend.com>> wrote:

    Thanks for review.

    Both problems should be fixed now
    https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330

    Do you see any other problems or a better way to fix this?

Your fix for DISCARD_EXCEPTION does not look right to me. It will discard all exceptions too early. For example: function test() {

    try {
        throw new Exception(1);
    } finally {
        try {
            try {
            } finally {
                return 42;
            }
        } finally {
            throw new Exception(2);
        }
    }

} test(); This will now not chain exception 1 and 2, because exception 1 is discarded at the return statement.

I thought about this, and I think that the current behavior is right. If we remove "throw new Exception(2);" the first exception is going to be discarded at "return 42;". I think we should do the same independently from the context. Why do you think Exception(1) should be kept?

I think this should be handled the same way we do the fast_call dispatch on return, i.e. when we pop the FAST_CALL from the loop var stack we should replace it with a DISCARD_EXCEPTION and then pop it after the finally. This should generate all the necessary DISCARD_EXCEPTION opcodes, and in the right order.

May be I'll commit the existing fix, and you'll try to implement this idea on top? Thanks. Dmitry.

Nikita