Skip to content

Two enums instead of preprocessor macros #10617

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

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Two enums instead of preprocessor macros #10617

merged 2 commits into from
Feb 21, 2023

Conversation

MaxKellermann
Copy link
Contributor

No description provided.

@@ -28,6 +28,11 @@ typedef struct _zend_stack {

#define STACK_BLOCK_SIZE 16

typedef enum {
ZEND_STACK_APPLY_TOPDOWN,
Copy link
Member

@devnexen devnexen Feb 19, 2023

Choose a reason for hiding this comment

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

Looks correct in general, just curiosity question : what led you to change the original values ?
It seems safe enough doing so, I do not think it will bite later on just curious..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reviewed all code locations where they are used, also checked the commit which added them (9c754da), and figured these were arbitrary values with no meaning. I also figured that zero (for which there is no macro) isn't a special case, for example by casting to bool somewhere or relying on zero-initialization (via memset() or bss).
Then I decided to not specifiy any explicit values at all, because that would indicate to the next guy that the values are important, when they really aren't. I decided for simplicity.

Copy link
Member

Choose a reason for hiding this comment

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

sounds fair.

Choose a reason for hiding this comment

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

I reviewed all code locations where they are used, also checked the commit which added them (9c754da), and figured these were arbitrary values with no meaning. I also figured that zero (for which there is no macro) isn't a special case, for example by casting to bool somewhere or relying on zero-initialization (via memset() or bss). Then I decided to not specifiy any explicit values at all, because that would indicate to the next guy that the values are important, when they really aren't. I decided for simplicity.

please - add this to commit message. and pretty please, with sugar on top - write BIG commit messages to explain next guy WHY this is done. You do a lot of good work, but not explain what and why was done.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! Can we enable -Wassign-enum on Clang (ASAN build on Cirrus)? This would provide a tangible benefit when converting to enums. Maybe there are other useful enum flags, there's a bunch of them for Clang.

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 21, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
@MaxKellermann
Copy link
Contributor Author

Can we enable -Wassign-enum on Clang (ASAN build on Cirrus)?

#10641

Though lots of code fails with this warning - and indeed, enums are (ab)used by the JIT and others as bit fields, which is not correct.

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 21, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
@iluuu1994 iluuu1994 merged commit bb07e20 into php:master Feb 21, 2023
@MaxKellermann MaxKellermann deleted the enums branch February 22, 2023 08:42
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
iluuu1994 pushed a commit to iluuu1994/php-src that referenced this pull request Feb 27, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
To find mistakes with enums.

See php#10617 (review)
dstogov added a commit to dstogov/php-src that referenced this pull request Apr 3, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This reverts commit bb07e20.
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.

None yet

5 participants