Skip to content

zend_string: Replace some macros by inline functions#21855

Open
TimWolla wants to merge 1 commit intophp:masterfrom
TimWolla:zend-string-demacro
Open

zend_string: Replace some macros by inline functions#21855
TimWolla wants to merge 1 commit intophp:masterfrom
TimWolla:zend-string-demacro

Conversation

@TimWolla
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I'm not sure if I can find Nikita's review comment again when I did those sorts of refactorings, but IIRC uppercase really should be reserved for macro functions was his opinion.

So maybe naming those inline functions using the "usual style" and defining the macros to just be those functions might be a better idea? And then we can slowly migrate the use of macros to functions and keep the macros just as BC shims for a while.

@TimWolla
Copy link
Copy Markdown
Member Author

defining the macros to just be those functions might be a better idea?

I really don't want more BC layers that will never get removed in practice. Uppercase functions are a bit unorthodox, but feel like the lesser evil in practice - particularly for this kind of function-like macro. At least until the internal API gets a proper cleanup with a predictable function naming and clear header dependencies.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 23, 2026

I don't have a strong opinion, so I'll wait to see what @iluuu1994 thinks about the uppercase functions.

@iluuu1994
Copy link
Copy Markdown
Member

I don't particularly care whether the function has an uppercase name or whether it's hidden behind a trivial macro, but I would not like to see the code churn of migrating the callers.

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.

3 participants