Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39581
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.46.29 as permitted sender)
DomainKey-Status: bad
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=subject:from:to:cc:in-reply-to:references:content-type:date
         :message-id:mime-version:x-mailer:content-transfer-encoding;
        b=GVtGA0N2OwxA1TBZAH26hjKqHqQgzlKhaHOeqjKSlGsBIE00lyXmBX9QQWaOjczx2q
         hK6iWNdi8VkLSBS9/SPQdUeK/EkUwxPstpKzXhHMejLU0//eO5nr5S+2zYinaW537je4
         pcYM/Rl8cSAkLuNgW3DIvpGfcA83i7TjXuOXU=
To: Marcus Boerger <helly@php.net>
Cc: internals@lists.php.net
In-Reply-To: <1124407685.20080803184656@marcus-boerger.de>
References: <1217769121.5923.10.camel@pena>
	 <1124407685.20080803184656@marcus-boerger.de>
Content-Type: text/plain; charset=utf-8
Date: Sun, 03 Aug 2008 19:06:24 -0300
Message-ID: <1217801185.5923.76.camel@pena>
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Const-correctness
From: felipensp@gmail.com (Felipe Pena)

Hello Marcus,

Em Dom, 2008-08-03 às 18:46 +0200, Marcus Boerger escreveu:
> 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.

That because the zend_hash_internal_pointer_reset_ex() called in
print_hash() and print_flat_hash() makes:
ht->pInternalPointer = ht->pListHead;


> -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'. 

The reason is that zend_print_zval_ex() uses:
zval_dtor(expr);


> Also note that not all of them use const for expr.
Oh right, I added in zend_print_zval_r_ex() now.


> 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'

Fixed!


> -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'.

Fixed.


> -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?

Fixed!


> -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'.

Fixed!


> -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'?

Because module->module_started had your value changed in the function.

>  
> -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.
> 

Hmmm, ok. I'll remove all the 'struct * const var', and to find other
cases where 'const struct * const var' and 'const var' can be suitable
too.


const thanks! :D

-- 
Regards,
Felipe Pena.