Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zend_enum: Rename try parameter to avoid conflict with C++ reserved word #15235

Closed
wants to merge 0 commits into from

Conversation

oplanre
Copy link

@oplanre oplanre commented Aug 4, 2024

Importing zend_enum.h breaks C++ files and needs to be bypassed with something like:

#define try try_
#include <zend_enum.h>
#undef try

This simple change fixes that.

@TimWolla TimWolla changed the title rename 'try' to 'allow_not_found' to avoid conflict with c++ reserved word zend_enum: Rename try parameter to avoid conflict with C++ reserved word Aug 4, 2024
@iluuu1994
Copy link
Member

iluuu1994 commented Aug 4, 2024

This is named after BackedEnum::tryFrom(), so there's some value in keeping the terminology consistent. I'd prefer to just add a _ suffix in the header file, but I'll go with the majority.

@cmb69
Copy link
Member

cmb69 commented Aug 4, 2024

Unless changing a parameter name is considered to constitute an API break, I suggest to apply to PHP-8.2.

I don't mind the name, but somewhat dislike trailing underscores.

@iluuu1994
Copy link
Member

Unless changing a parameter name is considered to constitute an API break, I suggest to apply to PHP-8.2.

Should be fine to apply to 8.2.

@oplanre oplanre closed this Aug 5, 2024
@oplanre oplanre reopened this Aug 5, 2024
@cmb69
Copy link
Member

cmb69 commented Aug 5, 2024

I've restarted the failing Windows CI run, that failed during install of additional components.

@cmb69
Copy link
Member

cmb69 commented Aug 6, 2024

@oplanre, did you close this deliberately? Are you planning to open a new PR instead?

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.

4 participants