Hello Dmitry,
please don't apply. The patch looks rather rough and untested (see below).
Also I really disagree to making the engine even more complex and adding
even more different behavior ways. That way we just introduce more errors
as we cannot test the engine in all its modes. We simply do not have the
infrastructure for that. And that means we will add even more bugs.
Further more I am missgin a description what you really do here and why we
need to do that. Ok Opcode caches have issues but so far all attempts to do
somethign about that have been blocked. Now this one apparently has a new
option. But as far as I can tell it simply allows to change the compiler to
an opcode friendly order. If that is all what it does than we should simply
drop all the options and do it anyway without flags.
unnoticed.
marcus
Friday, March 7, 2008, 12:36:58 PM, you wrote:
> Index: Zend/zend.c
> ===================================================================
> RCS file: /repository/ZendEngine2/zend.c,v
> retrieving revision 1.308.2.12.2.35.2.9
> diff -u -p -d -r1.308.2.12.2.35.2.9 zend.c
> --- Zend/zend.c 5 Mar 2008 13:34:12 -0000 1.308.2.12.2.35.2.9
> +++ Zend/zend.c 7 Mar 2008 11:34:14 -0000
> @@ -463,7 +463,7 @@ static void zend_set_default_compile_tim
> CG(asp_tags) = asp_tags_default;
> CG(short_tags) = short_tags_default;
> CG(allow_call_time_pass_reference) = ct_pass_ref_default;
> - CG(extended_info) = extended_info_default;
> + CG(compiler_options) = compiler_options_default;
> }
> /* }}} */
>
Why rename this variable? It still is an exetended information in the
compiler globals. It's full name would be compiler_globals_extended_info'
versus 'compiler_globals_compiler_options'.
> Index: Zend/zend_compile.c
> ===================================================================
> RCS file: /repository/ZendEngine2/zend_compile.c,v
> retrieving revision 1.647.2.27.2.41.2.46
> diff -u -p -d -r1.647.2.27.2.41.2.46 zend_compile.c
> --- Zend/zend_compile.c 3 Mar 2008 15:07:04 -0000 1.647.2.27.2.41.2.46
> +++ Zend/zend_compile.c 7 Mar 2008 11:34:15 -0000
[...]
> @@ -2588,7 +2577,7 @@ ZEND_API void zend_do_inheritance(zend_c
>
> if (ce->ce_flags & ZEND_ACC_IMPLICIT_ABSTRACT_CLASS && ce->type ==
> ZEND_INTERNAL_CLASS) {
> ce->ce_flags |= ZEND_ACC_EXPLICIT_ABSTRACT_CLASS;
> - } else {
> + } else if (!(ce->ce_flags & ZEND_ACC_IMPLEMENT_INTERFACES)) {
> zend_verify_abstract_class(ce TSRMLS_CC);
> }
> }
> @@ -2700,7 +2689,7 @@ ZEND_API zend_class_entry *do_bind_class
> }
> return NULL;
> } else {
> - if (!(ce->ce_flags & ZEND_ACC_INTERFACE)) {
> + if (!(ce->ce_flags &
> (ZEND_ACC_INTERFACE|ZEND_ACC_IMPLEMENT_INTERFACES))) {
> zend_verify_abstract_class(ce TSRMLS_CC);
> }
> return ce;
This looks like a change of behavior to the untrained eye.
[...]
> @@ -3238,54 +3235,36 @@ void zend_do_end_class_declaration(znode
>
> ce->line_end = zend_get_compiled_lineno(TSRMLS_C);
>
> - if (!(ce->ce_flags &
> (ZEND_ACC_INTERFACE|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS))
> - && ((parent_token->op_type != IS_UNUSED) ||
> (ce->num_interfaces > 0))) {
> + if (!(ce->ce_flags &
> (ZEND_ACC_INTERFACE|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS))) {
> zend_verify_abstract_class(ce TSRMLS_CC);
> - if (ce->parent || ce->num_interfaces) {
> + if (ce->ce_flags & ZEND_ACC_IMPLEMENT_INTERFACES) {
> do_verify_abstract_class(TSRMLS_C);
> }
> }
> - /* Inherit interfaces; reset number to zero, we need it for above
> check and
> - * will restore it during actual implementation. */
> - if (ce->num_interfaces > 0) {
> - ce->interfaces = NULL;
> - ce->num_interfaces = 0;
> - }
> CG(active_class_entry) = NULL;
> }
Again looks like change of behavior.
[...]
> Index: Zend/zend_compile.h
> ===================================================================
> RCS file: /repository/ZendEngine2/zend_compile.h,v
> retrieving revision 1.316.2.8.2.12.2.14
> diff -u -p -d -r1.316.2.8.2.12.2.14 zend_compile.h
> --- Zend/zend_compile.h 12 Feb 2008 00:20:33 -0000 1.316.2.8.2.12.2.14
> +++ Zend/zend_compile.h 7 Mar 2008 11:34:15 -0000
> @@ -143,6 +143,10 @@ typedef struct _zend_try_catch_element {
> /* deprecation flag */
> #define ZEND_ACC_DEPRECATED 0x40000
>
> +/* class implement interface(s) flag */
'Class implements interface(s) flag' Not the additional s.
Either way, this comment is completely useless. The name of the damn flag
tells me that it has something to do with classes implementing interfaces.
A comment is only uiseful if it adds information. And yes names should be
clear.
> +#define ZEND_ACC_IMPLEMENT_INTERFACES 0x80000
> +
> +
> char *zend_visibility_string(zend_uint fn_flags);
>
>
> +
> +#define ZEND_COMPILE_EXTENDED_INFO (1<<0)
> +#define ZEND_COMPILE_HANDLE_OP_ARRAY (1<<1)
> +#define ZEND_COMPILE_IGNORE_INTERNAL_FUNCTIONS (1<<2)
> +#define ZEND_COMPILE_IGNORE_INTERNAL_CLASSES (1<<3)
> +#define ZEND_COMPILE_DELAYED_BINDING (1<<4)
> +
> +#define ZEND_COMPILE_DEFAULT
> ZEND_COMPILE_HANDLE_OP_ARRAY
> +#define ZEND_COMPILE_DEFAULT_FOR_EVAL 0
> +
The above are all possible options. So why not name them
'ZEND_COMPILE_OPTION_*' and why no comments here?
[...]
> Index: Zend/zend_vm_opcodes.h
> ===================================================================
> RCS file: /repository/ZendEngine2/zend_vm_opcodes.h,v
> retrieving revision 1.42.2.17.2.1.2.4
> diff -u -p -d -r1.42.2.17.2.1.2.4 zend_vm_opcodes.h
> --- Zend/zend_vm_opcodes.h 31 Dec 2007 07:17:06 -0000
> 1.42.2.17.2.1.2.4
> +++ Zend/zend_vm_opcodes.h 7 Mar 2008 11:34:26 -0000
> @@ -18,135 +18,136 @@
> +----------------------------------------------------------------------+
> */
>
> -#define ZEND_NOP 0
[...]
> +#define ZEND_NOP 0
Please do not change the indent here or create the patch with -uwp as
otherwise one cannot tell where you added new stuff.
> Index: sapi/cgi/cgi_main.c
> ===================================================================
> RCS file: /repository/php-src/sapi/cgi/cgi_main.c,v
> retrieving revision 1.267.2.15.2.50.2.13
> diff -u -p -d -r1.267.2.15.2.50.2.13 cgi_main.c
> --- sapi/cgi/cgi_main.c 28 Feb 2008 00:51:56 -0000 1.267.2.15.2.50.2.13
> +++ sapi/cgi/cgi_main.c 7 Mar 2008 11:34:29 -0000
> @@ -1691,7 +1691,7 @@ consult the installation file that came
> break;
>
> case 'e': /* enable extended
> info output */
> - CG(extended_info) = 1;
> +
> CG(compiler_options) |= ZEND_COMPILE_EXTENDED_INFO;
> break;
>
> case 'f': /* parse file */
This does not work.
> Index: sapi/cli/php_cli.c
> ===================================================================
> RCS file: /repository/php-src/sapi/cli/php_cli.c,v
> retrieving revision 1.129.2.13.2.22.2.4
> diff -u -p -d -r1.129.2.13.2.22.2.4 php_cli.c
> --- sapi/cli/php_cli.c 3 Feb 2008 17:49:46 -0000 1.129.2.13.2.22.2.4
> +++ sapi/cli/php_cli.c 7 Mar 2008 11:34:29 -0000
> @@ -821,7 +821,7 @@ int main(int argc, char *argv[])
> break;
>
> case 'e': /* enable extended info output */
> - CG(extended_info) = 1;
> + CG(compiler_options) |=
> ZEND_COMPILE_EXTENDED_INFO;
> break;
>
> case 'F':
Does not work either.
> Index: sapi/milter/php_milter.c
> ===================================================================
> RCS file: /repository/php-src/sapi/milter/php_milter.c,v
> retrieving revision 1.14.2.2.2.3.2.2
> diff -u -p -d -r1.14.2.2.2.3.2.2 php_milter.c
> --- sapi/milter/php_milter.c 31 Dec 2007 07:17:18 -0000
> 1.14.2.2.2.3.2.2
> +++ sapi/milter/php_milter.c 7 Mar 2008 11:34:29 -0000
> @@ -1033,7 +1033,7 @@ int main(int argc, char *argv[])
> break;
>
> case 'e': /* enable extended info output */
> - CG(extended_info) = 1;
> + CG(compiler_options) |=
> ZEND_COMPILE_EXTENDED_INFO;
> break;
>
> case 'f': /* parse file */
And this does also not work.
The reason is that you did not update the table that allows those
additions. Given that fact I doubt that you tested what you did and hence
this is just a quick shot at something I fail to get.
Best regards,
Marcus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
