Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39574
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.94.56 as permitted sender)
Date: Sun, 3 Aug 2008 18:46:56 +0200
Reply-To: Marcus Boerger <helly@php.net>
Message-ID: <1124407685.20080803184656@marcus-boerger.de>
To: Felipe Pena <felipensp@gmail.com>
CC: internals@lists.php.net
In-Reply-To: <1217769121.5923.10.camel@pena>
References: <1217769121.5923.10.camel@pena>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Const-correctness
From: helly@php.net (Marcus Boerger)

Hello Felipe,

Sunday, August 3, 2008, 3:12:01 PM, you wrote:

> Hi all,

> I made a patch that add the const qualifier in several function
> parameters in Zend/* where it seems suitable (mostly the API).

> http://felipe.ath.cx/diff/const-ness.diff (5_3)

> HEAD comming soon in case of no objection.
> (And yes... I'm thinking commit it just when HEAD has been done too.)


Index: Zend/zend.c
===================================================================
-static void print_hash(HashTable *ht, int indent, zend_bool is_object TSRMLS_DC) /* {{{ */
+static void print_hash(HashTable * const ht, int indent, zend_bool is_object TSRMLS_DC) /* {{{ */
-static void print_flat_hash(HashTable *ht TSRMLS_DC) /* {{{ */
+static void print_flat_hash(HashTable * const ht TSRMLS_DC) /* {{{ */

Why not 'const HashTable * const ht', print doesn't sound like it should modify the table itself.

-ZEND_API int zend_print_zval(zval *expr, int indent) /* {{{ */
+ZEND_API int zend_print_zval(zval * const expr, int indent) /* {{{ */
-ZEND_API int zend_print_zval_ex(zend_write_func_t write_func, zval *expr, int indent) /* {{{ */
+ZEND_API int zend_print_zval_ex(const zend_write_func_t write_func, zval *expr, int indent) /* {{{ */
-ZEND_API void zend_print_flat_zval_r(zval *expr TSRMLS_DC) /* {{{ */
+ZEND_API void zend_print_flat_zval_r(zval * const expr TSRMLS_DC) /* {{{ */
-ZEND_API void zend_print_zval_r(zval *expr, int indent TSRMLS_DC) /* {{{ */
+ZEND_API void zend_print_zval_r(zval * const expr, int indent TSRMLS_DC) /* {{{ */
-ZEND_API void zend_print_zval_r_ex(zend_write_func_t write_func, zval *expr, int indent TSRMLS_DC) /* {{{ */
+ZEND_API void zend_print_zval_r_ex(const zend_write_func_t write_func, zval *expr, int indent TSRMLS_DC) /* {{{ */

In the same way these should be 'const zval * const expr'. Also note that not
all of them use const for expr.

Index: Zend/zend_API.c
=================================================================== 
-ZEND_API int zend_copy_parameters_array(int param_count, zval *argument_array TSRMLS_DC) /* {{{ */
+ZEND_API int zend_copy_parameters_array(int param_count, zval * const argument_array TSRMLS_DC) /* {{{ */

'const zval * const'

-ZEND_API int zend_get_object_classname(zval *object, char **class_name, zend_uint *class_name_len TSRMLS_DC) /* {{{ */
+ZEND_API int zend_get_object_classname(zval * const object, char **class_name, zend_uint *class_name_len TSRMLS_DC) /* {{{ */

Does not modify the zval container, so use 'const zval * const object'.
 
-static int parse_arg_object_to_string(zval **arg, char **p, int *pl, int type TSRMLS_DC) /* {{{ */
+static int parse_arg_object_to_string(zval ** const arg, char ** const p, int * const pl, int type TSRMLS_DC) /* {{{ */

This is named parse. Does it convert the zval in place? I rather think it might
create a copy of the zval and reduce the refcount in the original one. That
would explain wht you cannot do const zval here.

So at this point I guess we do that at a bunch of places and sadly we do not
have mutable. Maybe we need to do a lot of casting here to make refcount and
is_ref work as expected, that is not affect constness of a zval. Maybe some
compilers need 'const volatile' for both. If that works you need:
'const zval ** const arg'.

-static char *zend_parse_arg_impl(int arg_num, zval **arg, va_list *va, char **spec, char **error, int *severity TSRMLS_DC) /* {{{ */
+static char *zend_parse_arg_impl(int arg_num, zval **arg, va_list * const va, char ** const spec, char **error, int *severity TSRMLS_DC) /* {{{ */
-static int zend_parse_arg(int arg_num, zval **arg, va_list *va, char **spec, int quiet TSRMLS_DC) /* {{{ */
+static int zend_parse_arg(int arg_num, zval **arg, va_list * const va, char ** const spec, int quiet TSRMLS_DC) /* {{{ */

Shouldn't this be 'const char ** const spec'. After all we don't modify the actual string.
 
-static int zend_parse_va_args(int num_args, char *type_spec, va_list *va, int flags TSRMLS_DC) /* {{{ */
+static int zend_parse_va_args(int num_args, char *type_spec, va_list * const va, int flags TSRMLS_DC) /* {{{ */

Shouldn't this be 'const char *type_spec' for the same reason as above?

-ZEND_API int zend_parse_parameters_ex(int flags, int num_args TSRMLS_DC, char *type_spec, ...) /* {{{ */
+ZEND_API int zend_parse_parameters_ex(int flags, int num_args TSRMLS_DC, char * const type_spec, ...) /* {{{ */
-ZEND_API int zend_parse_parameters(int num_args TSRMLS_DC, char *type_spec, ...) /* {{{ */
+ZEND_API int zend_parse_parameters(int num_args TSRMLS_DC, char * const type_spec, ...) /* {{{ */
-ZEND_API int zend_parse_method_parameters(int num_args TSRMLS_DC, zval *this_ptr, char *type_spec, ...) /* {{{ */
+ZEND_API int zend_parse_method_parameters(int num_args TSRMLS_DC, zval * const this_ptr, char * const type_spec, ...) /* {{{ */
-ZEND_API int zend_parse_method_parameters_ex(int flags, int num_args TSRMLS_DC, zval *this_ptr, char *type_spec, ...) /* {{{ */
+ZEND_API int zend_parse_method_parameters_ex(int flags, int num_args TSRMLS_DC, zval * const this_ptr, char * const type_spec, ...) /* {{{ */

Shouldn't this be 'const char * const type_spec'.
For the method versions it should also be 'const zval * const this_ptr'.

-ZEND_API int zend_startup_module_ex(zend_module_entry *module TSRMLS_DC) /* {{{ */
+ZEND_API int zend_startup_module_ex(zend_module_entry * const module TSRMLS_DC) /* {{{ */

Why not 'const zend_module_entry * const module'?
 
-ZEND_API zend_module_entry* zend_register_internal_module(zend_module_entry *module TSRMLS_DC) /* {{{ */
+ZEND_API zend_module_entry* zend_register_internal_module(zend_module_entry * const module TSRMLS_DC) /* {{{ */

If only we had mutable...


Overall you do a ton of 'struct * const var' which only means that you are
going to copy the pointer explicitly. Now functions that use a pointer in a
loop and increment it cannot optimize the code anymore and are forced to use
an additional real variable on the stack. So unless you have a very smart
compiler, the result is an increased stack size. Generally speaking I prefer
const on the reight and mid (between *'s) only. But others prefer it to denote
that not even the passed in pointer gets modifed and this sometimes even makes
debugging easier.

> Since then, anyone have an objection?

See above, we might be able to add more const :-)


Best regards,
 Marcus