Use new ZSTR_INIT_LITERAL macro

[iluuu1994]

No description provided.

[bwoebi]

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

[TimWolla]

How did you perform and verify this change?

---

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

php-src/sapi/phpdbg/phpdbg.c

Line 232 in 5efd60e

zend_string *key = zend_string_init(ZEND_STRL("opcache.jit"), 1);

[iluuu1994]

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

[TimWolla]

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 😩

[iluuu1994]

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 🙂

[TimWolla]

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.

[Girgias]

LGTM.

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. :)

[iluuu1994]

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.

[iluuu1994]

Thanks for the review everyone 🙂