Repository navigation
Conversation
iluuu1994
left a comment
There was a problem hiding this comment.
Not a C++ person, but looks correct to me.
|
Merge with be09985. Thanks ! |
|
|
||
| #if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */ | ||
| #if defined(__cplusplus) | ||
| # define ZEND_STATIC_ASSERT(c, m) static_assert((c), m) |
There was a problem hiding this comment.
@psumbera @devnexen static_assert() was added in C++11 so this change breaks compilation with -std=c++98 (e.g. swig/swig#3255). If you're going to use static_assert() with C++, it should only be for C++11 and later.
There was a problem hiding this comment.
we did not put guards because php supports now C++11/17 minimum.
There was a problem hiding this comment.
honestly, if you have the choice, please drop C++98, std::move avoiding better dangling pointers is really worth the jump and that s not the only benefit ;)
There was a problem hiding this comment.
Is that also a requirement when building a third party extension module written in C++? That's the situation where we hit this (rather than when building PHP itself as in the linked issue).
If so, where is this requirement documented?
There was a problem hiding this comment.
not documented per say but PHP had updated the C++ build here.
There was a problem hiding this comment.
Sadly I don't really have the choice (though personally I tend to agree). SWIG is a tool to generate bindings for various langauges, including PHP, and those bindings are very widely used so we're pretty conservative on raising such requirements.
I can argue for raising it specifically for SWIG/PHP if PHP itself rejects compiling with older C++ versions, though it'd be easier to argue if this requirement were documented properly, and also useful to know how evolves with each PHP version. I'd think documenting this would also be helpful for other users.
There was a problem hiding this comment.
yes good point about documenting
Fixes: #19169