Skip to content

Conversation

DanielEScherzer
Copy link
Member

Currenty, if @refcount is omitted, it is assumed to be 0 for scalar return types and "N" otherwise. On the other hand, if @refcount is provided, for a scalar return type it must be 0, and for a non-scalar return type it must not be 0. In other words, the ref count will be 0 if and only if the return type is scalar, regardless of whether the @refcount tag is used.

In that case, adding @refcount 0 does nothing, and since its presence suggests it does something (why would a developer add code that does nothing?) it is confusing and should be removed. As it happens, there are currently no uses of @refcount 0 in php-src, but why should we allow future uses?

Removing this support also allows simplifying the code a bit.

In the process, I also renamed some of the constants for clarity.

Currenty, if `@refcount` is omitted, it is assumed to be 0 for scalar return
types and "N" otherwise. On the other hand, if `@refcount` is provided, for a
scalar return type it *must* be 0, and for a non-scalar return type it
*must not* be 0. In other words, the ref count will be 0 if and only if the
return type is scalar, regardless of whether the `@refcount` tag is used.

In that case, adding `@refcount 0` does nothing, and since its presence
suggests it does something (why would a developer add code that does nothing?)
it is confusing and should be removed. As it happens, there are currently no
uses of `@refcount 0` in php-src, but why should we allow future uses?

Removing this support also allows simplifying the code a bit.

In the process, I also renamed some of the constants for clarity.
@DanielEScherzer
Copy link
Member Author

I didn't include this in #16291 since it is technically a functional change, but it shouldn't actually affect much

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea makes sense to me, but I had two nits

@DanielEScherzer
Copy link
Member Author

After this merges, @kocsismate would you mind taking a look at #16291 for some other improvements?

@kocsismate kocsismate merged commit be69262 into php:master Nov 30, 2024
11 checks passed
@DanielEScherzer DanielEScherzer deleted the refcount-scalar branch November 30, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants