Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Zend/zend_types.h: deprecate zend_bool, zend_intptr_t, zend_uintptr_t#10597

Merged
Girgias merged 1 commit into
php:masterphp/php-src:masterfrom
CM4all:intptrCM4all/php-src:intptrCopy head branch name to clipboard
Feb 18, 2023
Merged

Zend/zend_types.h: deprecate zend_bool, zend_intptr_t, zend_uintptr_t#10597
Girgias merged 1 commit into
php:masterphp/php-src:masterfrom
CM4all:intptrCM4all/php-src:intptrCopy head branch name to clipboard

Conversation

@MaxKellermann

Copy link
Copy Markdown
Contributor

These types are standard C99. For compatibility with out-of-tree extensions, keep the typedefs in main/php.h.

Comment thread Zend/zend_types.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be probably deprecated as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand zend_bool, zend_*inptr_t were definitions from a time when C99 wasn't required; but unsigned char has always been available, everywhere. Therefore I figured this is not a compatibility definition, but .... I don't know. This doesn't make any sense to me, but I suggested deprecating only those I understood.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Apparently, zend_uchar isn't used as an unsigned char ("char" as in "ASCII character" - the C nomenclature is confusing), but as small unsigned integer. This is confusing. I'd recommend using uint_least8_t everywhere instead, to clearly indicate that this is about small integers and not ASCII characters.

Comment thread main/php.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if used, they should emit a compiler warning, also I would add something like "and will be removed in PHP 9.0"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added ZEND_ATTRIBUTE_DEPRECATED, but there appears to be no way to attach a message to this GCC attribute.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GCC should warn implicitly when -Wno-deprecated-declarations is not passed

php-src CI should not pass if any GCC warning is produced

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I thought you meant that GCC should write "will be removed in 9" in the message. In C++, the deprecated attribute can be annotated with a message that is printed (https://en.cppreference.com/w/cpp/language/attributes/deprecated), but not the proprietary GCC attribute.
Will add the 9.0 comment.

@MaxKellermann MaxKellermann force-pushed the intptr branch 2 times, most recently from 034cb2e to 3544ec4 Compare February 15, 2023 13:17
These types are standard C99.  For compatibility with out-of-tree
extensions, keep the typedefs in main/php.h.
@MaxKellermann

Copy link
Copy Markdown
Contributor Author

AppVeyor failure because ZEND_ATTRIBUTE_DEPRECATED doesn't work with typedefs on Windows (MSVC).
I removed the attribute for now; maybe we need a typedef-specific macro for the deprecated attribute that's empty on MSVC?

@mvorisek

Copy link
Copy Markdown
Contributor

AppVeyor failure because ZEND_ATTRIBUTE_DEPRECATED doesn't work with typedefs on Windows (MSVC). I removed the attribute for now; maybe we need a typedef-specific macro for the deprecated attribute that's empty on MSVC?

https://stackoverflow.com/questions/4995868/deprecate-typedef

@MaxKellermann

Copy link
Copy Markdown
Contributor Author

Oh hooray, GCC wants the attribute at the end after the name, and MSVC wants it between typedef and the type.

@Girgias Girgias merged commit 413844d into php:master Feb 18, 2023
simpletoimplement added a commit to simpletoimplement/igbinary-igbinary that referenced this pull request Feb 18, 2023
@MaxKellermann MaxKellermann deleted the intptr branch February 21, 2023 15:32
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.