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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4bfb9c
Zend/zend_compile: convert `memoize_mode` macros to enum
MaxKellermann Feb 18, 2023
2d47d13
Zend/zend_stack: convert `ZEND_STACK_APPLY_*` macros to enum
MaxKellermann Feb 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ void zend_init_compiler_data_structures(void) /* {{{ */

CG(encoding_declared) = 0;
CG(memoized_exprs) = NULL;
CG(memoize_mode) = 0;
CG(memoize_mode) = ZEND_MEMOIZE_NONE;
}
/* }}} */

Expand Down Expand Up @@ -2426,13 +2426,9 @@ static void zend_emit_jmp_null(znode *obj_node, uint32_t bp_type)
zend_stack_push(&CG(short_circuiting_opnums), &jmp_null_opnum);
}

#define ZEND_MEMOIZE_NONE 0
#define ZEND_MEMOIZE_COMPILE 1
#define ZEND_MEMOIZE_FETCH 2

static void zend_compile_memoized_expr(znode *result, zend_ast *expr) /* {{{ */
{
int memoize_mode = CG(memoize_mode);
const zend_memoize_mode memoize_mode = CG(memoize_mode);
if (memoize_mode == ZEND_MEMOIZE_COMPILE) {
znode memoized_result;

Expand Down Expand Up @@ -9184,7 +9180,7 @@ static void zend_compile_assign_coalesce(znode *result, zend_ast *ast) /* {{{ */
/* Remember expressions compiled during the initial BP_VAR_IS lookup,
* to avoid double-evaluation when we compile again with BP_VAR_W. */
HashTable *orig_memoized_exprs = CG(memoized_exprs);
int orig_memoize_mode = CG(memoize_mode);
const zend_memoize_mode orig_memoize_mode = CG(memoize_mode);

zend_ensure_writable_variable(var_ast);
if (is_this_fetch(var_ast)) {
Expand Down
8 changes: 7 additions & 1 deletion Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ typedef struct _zend_ini_entry zend_ini_entry;
typedef struct _zend_fiber_context zend_fiber_context;
typedef struct _zend_fiber zend_fiber;

typedef enum {
ZEND_MEMOIZE_NONE,
ZEND_MEMOIZE_COMPILE,
ZEND_MEMOIZE_FETCH,
} zend_memoize_mode;

struct _zend_compiler_globals {
zend_stack loop_var_stack;

Expand Down Expand Up @@ -129,7 +135,7 @@ struct _zend_compiler_globals {

zend_stack delayed_oplines_stack;
HashTable *memoized_exprs;
int memoize_mode;
zend_memoize_mode memoize_mode;

void *map_ptr_real_base;
void *map_ptr_base;
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_stack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ ZEND_API void zend_stack_apply(zend_stack *stack, int type, int (*apply_function
}


ZEND_API void zend_stack_apply_with_argument(zend_stack *stack, int type, int (*apply_function)(void *element, void *arg), void *arg)
ZEND_API void zend_stack_apply_with_argument(zend_stack *stack, zend_stack_apply_direction type, int (*apply_function)(void *element, void *arg), void *arg)
{
int i;

Expand Down
10 changes: 6 additions & 4 deletions Zend/zend_stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

Copy link

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.

ZEND_STACK_APPLY_BOTTOMUP,
} zend_stack_apply_direction;

BEGIN_EXTERN_C()
ZEND_API void zend_stack_init(zend_stack *stack, int size);
ZEND_API int zend_stack_push(zend_stack *stack, const void *element);
Expand All @@ -39,11 +44,8 @@ ZEND_API void zend_stack_destroy(zend_stack *stack);
ZEND_API void *zend_stack_base(const zend_stack *stack);
ZEND_API int zend_stack_count(const zend_stack *stack);
ZEND_API void zend_stack_apply(zend_stack *stack, int type, int (*apply_function)(void *element));
ZEND_API void zend_stack_apply_with_argument(zend_stack *stack, int type, int (*apply_function)(void *element, void *arg), void *arg);
ZEND_API void zend_stack_apply_with_argument(zend_stack *stack, zend_stack_apply_direction type, int (*apply_function)(void *element, void *arg), void *arg);
ZEND_API void zend_stack_clean(zend_stack *stack, void (*func)(void *), bool free_elements);
END_EXTERN_C()

#define ZEND_STACK_APPLY_TOPDOWN 1
#define ZEND_STACK_APPLY_BOTTOMUP 2

#endif /* ZEND_STACK_H */