Inline functions instead of macros #6030
Conversation
|
Nice job~ Although we upgraded to C99, can we make the code style consistent? I really hope that the code style has a unified standard. I personally hope to continue to use the original coding way, |
Zend/zend.c
Outdated
| } | ||
| /* }}} */ | ||
|
|
||
| extern ZEND_API inline size_t zend_print_variable(zval *var); |
There was a problem hiding this comment.
No use of extern inline in php-src please.
There was a problem hiding this comment.
I must say I'm rather confused as to what is the difference between extern inline and static inline is, I used static inlines initially but when I talked to @morrisonlevi in R11 he preferred extern inline as apparently, IIRC, the semantics differ for static inline depending on the compiler. But I'm having a hard time finding relevant information about it.
There was a problem hiding this comment.
Yes, @morrisonlevi has opinions on this topic, but in php-src we do not use extern inline. It should be static inline or static zend_always_inline.
There was a problem hiding this comment.
Right, I will change them accordingly then.
There was a problem hiding this comment.
For what it is worth, it's not my opinion; it's actually the C99 and newer standard. The standards aren't freely available so it's hard to link, but here's GNU's explanation (with weird macros, but hopefully you can see through them): https://www.gnu.org/software/gnulib/manual/html_node/extern-inline.html. Here's a Stack Overflow answer as well, though I don't know about the C++ part: https://stackoverflow.com/a/216546.
Of course, compilers support static inline functions whether they are still standard or not, so I do acknowledge it is somewhat pedantic. The main reason I care is that you cannot use a static inline function inside of a plain inline function, which makes this non-standard use proliferate into my own codebase.
Now, for the opinion part: in my opinion, all usages of static inline should be converted into inline + extern inline and all usages of static zend_always_inline should remain as-is. In my opinion this strikes a nice balance between standard compliance and pragmatism.
For what it is worth, here are some rough stats on usage:
$ grep -Ir "static zend_always_inline" Zend | wc -l
416
$ grep -Ir "static inline" Zend | wc -l
83
Girgias
force-pushed
the
inline-functions-instead-of-macros
branch
from
e85289f to
97e7f04
Compare
Girgias
force-pushed
the
inline-functions-instead-of-macros
branch
from
97e7f04 to
39770bc
Compare
Girgias
force-pushed
the
inline-functions-instead-of-macros
branch
from
39770bc to
4b849f3
Compare
| END_EXTERN_C() | ||
|
|
||
| /* output support */ | ||
| // TODO Convert to inline functions? |
There was a problem hiding this comment.
Anything spelled UPPERCASE should stay a macro.
|
TBH I'm not completely clear on what advantage we get from doing this, for these kind of "trivial" macros, that just forward to some underlying implementation. Does this have something to do with C++ code and namespacing? |
|
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken. |
5 participants
This PR converts various macros in inline functions.
Notable omissions of this pass: