Skip to content

Generated arginfo header files: replace sizeof() with strlen() #16142

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Generated arginfo header files: replace sizeof() with strlen() #16142

wants to merge 1 commit into from

Conversation

DanielEScherzer
Copy link
Member

The coding conventions call for using strlen() on constant strings, but not refactoring existing uses of sizeof() unless there is existing refactoring taking place. I have recently sent a few patches that refactored/simplified some of the arginfo headers, but this patch touches more of them.

The generated headers present a special case - until they are updated to use strlen(), new additions will be forced to use sizeof(), since that is what the generator script produces. Let's just make the change now.
Girgias
Girgias approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I'm fine with this change.
@nikic
Copy link
Member

nikic commented Sep 30, 2024

What's the motivation for this mass change? I wouldn't go out of my way to use sizeof-1 on the premise that "the compiler will optimize it", but if there's one place for that pattern, generated code would be it.
@bwoebi
Copy link
Member

bwoebi commented Sep 30, 2024

Agreeing with Nikita here. It's a fine assumption to have in regular code, but not in widely used macros and generated code.
@DanielEScherzer
Copy link
Member Author

DanielEScherzer commented Sep 30, 2024 •
edited
Loading

What's the motivation for this mass change? I wouldn't go out of my way to use sizeof-1 on the premise that "the compiler will optimize it", but if there's one place for that pattern, generated code would be it.

When I first got interested in PHP internal development, I wrote an extension (https://github.com/DanielEScherzer/CustomCast/) and since I used the stub generation script, I thought that the preference was for sizeof() rather than strlen() - it wasn't until later when I saw the core coding conventions that I realized I had gotten it backwards (DanielEScherzer/CustomCast@5ca77bf). I would assume that learning to write an extension is how some others first dip their toes into PHP internal development, and suggest that the script used to generate files produce code that aligns with the code conventions
@DanielEScherzer
Generated arginfo header files: replace sizeof() with strlen()
425e7cc 
The coding conventions call for using `strlen()` on constant strings, but not
refactoring existing uses of `sizeof()` unless there is existing refactoring
taking place. I have recently sent a few patches that refactored/simplified
some of the arginfo headers, but this patch touches more of them.

The generated headers present a special case - until they are updated to use
`strlen()`, new additions will be forced to use `sizeof()`, since that is what
the generator script produces. Let's just make the change now.
@DanielEScherzer
Copy link
Member Author

I rebased the patch, but let me know if I should just abandon this instead
dstogov
dstogov reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I don't see a big reason in this renaming.
derickr
derickr requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

I also don't think we should do this.
@bukka
Copy link
Member

bukka commented Dec 12, 2024

Yeah I don't see much point in it too.
@iluuu1994
Copy link
Member

I think we have decided against this. Thanks nonetheless.
@iluuu1994 iluuu1994 closed this Feb 7, 2025
@DanielEScherzer DanielEScherzer deleted the arginfo-strlen branch February 16, 2025 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI break Category: Build System Category: Engine Extension: bcmath Extension: curl Extension: date Extension: dom Extension: enchant Extension: ffi Extension: ftp Extension: hash Extension: intl Extension: ldap Extension: libxml Extension: mysqli Extension: odbc Extension: openssl Extension: pdo (core) Extension: pdo_dblib Extension: pdo_firebird Extension: pdo_mysql Extension: pdo_odbc Extension: pdo_pgsql Extension: pdo_sqlite Extension: pgsql Extension: phar Extension: random Extension: reflection Extension: shmop Extension: snmp Extension: soap Extension: sodium Extension: spl Extension: sqlite3 Extension: standard Extension: tidy Extension: tokenizer Extension: xml Extension: xmlreader Extension: xsl Extension: zend_test Extension: zip
8 participants
@DanielEScherzer @nikic @bwoebi @bukka @iluuu1994 @derickr @dstogov @Girgias