Hi,
I have written a follow-up to my previous 'zstrict' PR. It is now
focused on a simple mechanism to detect API violations :
https://github.com/php/php-src/pull/1508
This version just implements the mechanism and applies it to the
zend_string val/len/h elements. More details in the PR.
Please comment first. Then, when we're OK on the feature, we may talk
about the target version.
Regards
François
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
Hi Francois,
-----Original Message-----
From: François Laupretre [mailto:francois@php.net]
Sent: Tuesday, September 8, 2015 12:41 AM
To: PHP internals internals@lists.php.net
Cc: Ferenc Kovacs tyra3l@gmail.com; Anthony Ferrara
ircmaxell@gmail.com
Subject: [PHP-DEV] strict-api - Proposing a tool to check and enforce
encapsulationHi,
I have written a follow-up to my previous 'zstrict' PR. It is now focused on a
simple mechanism to detect API violations :https://github.com/php/php-src/pull/1508
This version just implements the mechanism and applies it to the zend_string
val/len/h elements. More details in the PR.Please comment first. Then, when we're OK on the feature, we may talk about
the target version.
Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC. Especially when the idea expands over zend_string API later, evermore devs and code will be forced by the rules, so it is essential to ensure the majority wants it in first place. And, I wouldn't hurry it into 7.0.
Regards
Anatol
Hi
2015-09-12 17:57 GMT+02:00 Anatol Belski anatol.php@belski.net:
Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC. Especially when the idea expands over zend_string API later, evermore devs and code will be forced by the rules, so it is essential to ensure the majority wants it in first place. And, I wouldn't hurry it into 7.0.
I agre, but I don't see any chance of this making it into the 7.0
branch at current state, so I would recommed targetting 7.1 at
earliest
--
regards,
Kalle Sommer Nielsen
kalle@php.net
Le 12/09/2015 17:57, Anatol Belski a écrit :
Personally I'd be voting yes on the principle. However as the topic brought controversial opinions in the past, IMHO it is better to solve it through an RFC.
Especially when the idea expands over zend_string API later, evermore
devs and code will be forced by the rules, so it is essential to ensure
the majority wants it in first place. And, I wouldn't hurry it into 7.0.
OK. I'll write an RFC targeting 7.1. You're right : as long as it was an
optional tool, incorporating it without an RFC could be discussed, but
deciding to perform travis checks in zstrict mode requires a vote.
About the option name, I proposed some in the PR in reply to Ferenc's
suggestions. Which one do you prefer ?
Regards
François
Am 08.09.2015 um 00:41 schrieb François Laupretre francois@php.net:
Hi,
I have written a follow-up to my previous 'zstrict' PR. It is now focused on a simple mechanism to detect API violations :
https://github.com/php/php-src/pull/1508
This version just implements the mechanism and applies it to the zend_string val/len/h elements. More details in the PR.
Please comment first. Then, when we're OK on the feature, we may talk about the target version.
Regards
François
While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.
Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)
While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.
Thanks for your efforts here though!
Bob
Hi Bob,
Le 13/09/2015 00:44, Bob Weinand a écrit :
While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.
Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.
I was probably not clear enough : you will never debug code
configured/compiled in zstrict mode. This mode is reserved for
compile-time checks only. Once you have checked that everything compiles
fine in zstrict mode, the next step is to 'make clean' and throw the
compilation result away. You can 'make test' before, if you want, but
you will never distribute nor debug code compiled in zstrict mode.
The same with travis: the zstrict flag can be set only because the
resulting code will never be distributed nor used for debugging.
So, there's no question of debug macro nor .gdbinit. When debugging your
code, you'll keep using the usual field names
(fbc->op_array.filename->val for instance).
Regards
François
Am 13.09.2015 um 03:24 schrieb François Laupretre francois@php.net:
Hi Bob,
Le 13/09/2015 00:44, Bob Weinand a écrit :
While I don't disagree with the idea of making accidental direct access harder, I see a particular disadvantage for debugging.
Nobody will like to write "p fbc->op_array.filename->zstrict_field_zend_string_val" (I hope I got it right?) just to figure out where the targeted op_array is located. [At least lldb only shows one character of the value when just printing the zend_string, due to the struct hack...]
Now you may say macro? Well, honestly, I don't usually use macros (as .gdbinit isn't compatible with lldb and lldb is more stable than gdb on OS X)While it brings a minor guarantee that the ZSTR_*() API is really used, it makes debugging much harder; so I prefer to not merge this patch.
I was probably not clear enough : you will never debug code configured/compiled in zstrict mode. This mode is reserved for compile-time checks only. Once you have checked that everything compiles fine in zstrict mode, the next step is to 'make clean' and throw the compilation result away. You can 'make test' before, if you want, but you will never distribute nor debug code compiled in zstrict mode.
The same with travis: the zstrict flag can be set only because the resulting code will never be distributed nor used for debugging.
So, there's no question of debug macro nor .gdbinit. When debugging your code, you'll keep using the usual field names (fbc->op_array.filename->val for instance).
Regards
François
Well, that either just makes workflow unnecessarily complex or many devs just won't care too much about it… and these who do may be annoyed to fix others code before it's useful for themselves...
(Also, I don't think like some devs will be particularly eager to recompile before each push … it doesn't take just one minute on every machine)
Additionally, extension devs… They'd have to switch target php install while coding, just to check against zstrict install and no zstrict. Well… that just begins to become insane.
Sorry, but making zstrict optional is just plain annoying of developers...
Bob
Hi,
Le 13/09/2015 10:41, Bob Weinand a écrit :
Well, that either just makes workflow unnecessarily complex or many devs just won't care too much about it… and these who do may be annoyed to fix others code before it's useful for themselves...
(Also, I don't think like some devs will be particularly eager to recompile before each push … it doesn't take just one minute on every machine)
I'm not sure I understand what you mean. I agree that running travis
checks in zstrict mode introduces additional constraints for core
developers. But, I would say that's the price to pay to maintain some
level of isolation/encapsulation. I know we don't always agree on the
importance of encapsulation but zstrict is all about it.
The decision we need to make is whether core devs are ready to accept
these constraints to maintain some level of isolation in the code. As I
already said, zend_string is not the target. The real target is to
eliminate tight coupling on zend_hash and other complex APIs.
Additionally, extension devs… They'd have to switch target php install while coding, just to check against zstrict install and no zstrict. Well… that just begins to become insane.
Extension devs won't have to switch between zstrict and non-zstrict PHP
installs. If you look at the code, you will see that there are two
configure flags, one for the core, one for extensions. Extensions will
always be configured and compiled against a non-zstrict compiled core,
and zstrict mode will be set in the extension configure command. It is
different from other configure flags inherited from the core. So, once
again, you will never keep nor use a zstrict-compiled install tree
(which would be insane, I agree).
Sorry, but making zstrict optional is just plain annoying of developers...
??
Regards
François