Skip to content

Fix ZEND_STATIC_ASSERT for -std=c++17 - #19175

Closed
psumbera wants to merge 1 commit into
php:PHP-8.3from
psumbera:my-fix-branch
Closed

psumbera wants to merge 1 commit into
php:PHP-8.3from
psumbera:my-fix-branch

Conversation

@psumbera

@psumbera psumbera commented Jul 18, 2025 •

Copy link
Copy Markdown
Contributor

Fixes: #19169

@devnexen
devnexen requested a review from iluuu1994 July 18, 2025 16:26

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a C++ person, but looks correct to me.

@devnexen

Copy link
Copy Markdown
Member

Merge with be09985. Thanks !

@devnexen devnexen closed this Jul 18, 2025
Comment thread Zend/zend_portability.h

#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */
#if defined(__cplusplus)
# define ZEND_STATIC_ASSERT(c, m) static_assert((c), m)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we did not put guards because php supports now C++11/17 minimum.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ;)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not documented per say but PHP had updated the C++ build here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes good point about documenting

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.

Zend/zend_portability.h:789:49: error: expected constructor, destructor, or type conversion before '(' token

4 participants