Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion Zend/zend_portability.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,9 @@ extern "C++" {
/** @deprecated */
#define ZEND_CGG_DIAGNOSTIC_IGNORED_END ZEND_DIAGNOSTIC_IGNORED_END

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

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

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

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

#elif defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */
# define ZEND_STATIC_ASSERT(c, m) _Static_assert((c), m)
#else
# define ZEND_STATIC_ASSERT(c, m)
Expand Down
Loading