Good idea - though I wished these literal zend_strings could just automagically become known_strings.

How did you perform and verify this change?

There's also existing places that use ZEND_STRL that should likely also be adjusted:

@TimWolla I used a naive global search (zend_string_init("), and changed it manually via vim macro. Ah, so there is an existing macro after all, I was looking but couldn't find it. The new macro seems more readable and discoverable though.

The new macro seems more readable and discoverable though.

I'm not a fan of macros at all, but at least the new one behaves like a function, yes.

I wanted to check the changes with Coccinelle:

@@
constant s;
expression e;
@@
- zend_string_init((s), strlen(s), (e))
+ ZSTR_INIT_LITERAL(s, e)
@@
constant s;
expression e;
@@
- zend_string_init((s), (sizeof(s)-1), (e))
+ ZSTR_INIT_LITERAL(s, e)
@@
constant s;
expression e;
@@
- zend_string_init(ZEND_STRL(s), e)
+ ZSTR_INIT_LITERAL(s, e)

But it appears that Coccinelle gets confused by the ZPP macros for some reason. Bummer 😩

But it appears that Coccinelle gets confused by the ZPP macros for some reason. Bummer 😩

Yeah, the ZEND_STRL macro is weird one, substituting two parameters...

Coccinelle

I remember you mentioning it and couldn't remember the name. I'll look into it so I can use it in the future. Thanks 🙂

I remember you mentioning it and couldn't remember the name. I'll look into it so I can use it in the future. Thanks slightly_smiling_face

Basic usage would be:

spatch --in-place -sp_file foo.cocci --include-headers-for-types --recursive-includes --dir ext/

However comparing the result with your PR does not make all the changes. Specifically looking at the change to bcscale it does nothing. However if I remove the ZEND_PARSE_PARAMETERS block it works properly. It also works if I add a semicolon after each line in the ZEND_PARSE_PARAMETERS block. It also works if I clear out all the relevant ZPP macros with the following macro file:

#define ZEND_PARSE_PARAMETERS_START(flags, min_num_args, max_num_args) ;
#define ZEND_PARSE_PARAMETERS_END(failure) ;
#define Z_PARAM_OPTIONAL() ;
#define Z_PARAM_LONG_EX(dest, is_null, check_null, deref) ;

and then use spatch --macro-file=test.macro --in-place -sp_file test.cocci --include-headers-for-types --recursive-includes ext/bcmath/bcmath.c

So something about the macro causes Coccinelle to misparse the function and thus ignoring it.

Didn't know about the other bizarre macro. And having those become known strings seems also interesting, especially as a lot are INI settings, but that can be done as a follow up. :)

I don't know how that would work though. Known strings are part of the known_strings static list. Adding them to the list at runtime would essentially just model string interning.

Thanks for the review everyone 🙂