Use RETURN_THROWS() macro when an exception is thrown

[kocsismate]

Let's introduce a macro in order to indicate that an exception was thrown previously as proposed by Nikita in #4994 (review) (Click "Show resolved").

It made sense for me to use this macro even when the exception was thrown one line before, but I can also change them back to normal returns.

[nikic]

Would be great if we can get this used comprehensively at least for zpp return, to avoid coding style inconsistencies. Will probably take a while until this is used everywhere it should be used...

[kocsismate]

My plan is to do the migration for ZPP and for some exceptions (after zend_type_error(), zend_value_error(), zend_throw_error(), and zend_throw_exception()) at least in Zend + the most extensions. I still have a week until my vacation ends... 😎

If you agree, I can commit the most straightforward changes directly.

[nikic]

Yeah, committing those directly is fine. Please add a (void) return_value; into the macro though, to make sure that it's only used in contexts that actually have a return value. That would be my main concern when it comes to mass changes.

[kocsismate]

Thanks for the hint, I was actually wondered how to avoid using this macro multiple times in the same code branch.

So the final version of RETURN_THROWS() should be as follows, right?
do { ZEND_ASSERT(EG(exception)); return (void) return_value; } while (0)

[nikic]

do { ZEND_ASSERT(EG(exception)); (void) return_value; return; } while (0)

It would be fairly unusual to return a void value, I'm not sure it's permitted at all.

[kocsismate]

Ok, I didn't know it's possible in this form (btw my version also worked for me).

[nikic]

zend_fetch_resource() is another candidate for mass conversion.

[kocsismate]

Thanks for the idea! I was about to ask you for suggestions, so feel free to write me should a candidate come to your mind.

Would try_convert_to_string() also qualify as a good target? As far as I saw the implementation, it is, but I want to be 100% sure.

[nikic]

@kocsismate Yeah, try_convert_to_string() throws iff it returns null.

[kocsismate]

Just one more question: should I also include zend_fetch_resource_ex() and zend_fetch_resource2() as well, right? They seem to be appropriate as well, but I can't determine their purpose/why they also exist.

[nikic]

Yes. _ex allows NULL, 2 checks for two resource types (generally persistent + non-persistent). In all cases the deciding factor whether it throws or not is whether the resource name is passed.

[kocsismate]

Thanks!

In all cases the deciding factor whether it throws or not is whether the resource name is passed.

Yep, I noticed this after I finished the migration for zend_fetch_resource() locally, so I'll double check their name before committing.