Streams constify API change in master leads to compilation warning for extensions

My understanding is that the const triggers some optimizations because the
compiler better understands how the variables are used.

I'm not sure what degree of a difference it makes, but I'm always a fan of
using const if it's appropriate.

Perhaps your effort is better spent modifying the extensions to not emit
warnings?

I would be concerned if there were compilation errors rather than warnings.

--
William Bartlett
College of Engineering | Cornell University '14
240-432-5189

Hi,

I was wondering why stream API has been changed in this commit:

https://github.com/php/php-src/commit/92d27ccb0574f901a107409a7fec92888fa2b82f

Basically all char pointers have been constified. The thing is that this
commit leads to compilation warning for many extensions.

In my case I use php_stream_locate_url_wrapper and want to compile my
extension (fann) without any warnings. Because this change is in master and
not in 5.5-, I will have to add some ifdefs and cast it for 5.6+.

The thing is that php_stream_locate_url_wrapper and other stream fuctions
are often used for function parameters from zend_parse_parameters which are
just char *. Then the values have to be casted.

I understand that APIs should be improved and sometimes changes are
necessary but in this case I have to ask a question. Was this change worthy
to compilation warnings for many extensions?

If not would it be possible to revert it?

Thanks

Jakub

Le 02/10/2013 20:41, Jakub Zelenka a écrit :

Hi,

I was wondering why stream API has been changed in this commit:

https://github.com/php/php-src/commit/92d27ccb0574f901a107409a7fec92888fa2b82f

Basically all char pointers have been constified. The thing is that this
commit leads to compilation warning for many extensions.

In my case I use php_stream_locate_url_wrapper and want to compile my
extension (fann) without any warnings. Because this change is in master and
not in 5.5-, I will have to add some ifdefs and cast it for 5.6+.

The thing is that php_stream_locate_url_wrapper and other stream fuctions
are often used for function parameters from zend_parse_parameters which are
just char *. Then the values have to be casted.

I understand that APIs should be improved and sometimes changes are
necessary but in this case I have to ask a question. Was this change worthy
to compilation warnings for many extensions?

I don't see any problem with this change.

When a (char *) is expected (which means, give me a pointer to something
I can change), you can only give a (char *). So a (const char *) raises
a warning.

When a (const char *) is expected (which means, give me a pointer to
something I won't change), you can give a (char *) or a (const char *),
both are accepted without any warning.

In fann you had a (char *),
I propose you to remove the cast to (const char *) which raise a warning
with 5.5 and is not useful in master.

In the opposite way a cast from const to not-const have of course no sense.

So, yes we need to add more const in php API, as this is the correct way
to properly fix various build warnings.

About the linked commit, I think it would be useful in 5.5 (as this
don't break ABI).

Note, a string (such as "some name") is consider by gcc as a (char *)
but as a (const char *) by g++ (except if -fno-const-strings is used).

Ex :
http://git.php.net/?p=php-src.git;a=commitdiff;h=f7eff9cd41e0b996af9a0a01d3c5f8fdd8b7fa60
This constify option of exception functions fixes warning (at least) in
xmldiff extension (written in C++) which use const strings.

Remi.

PS. is a warning free build a dream ? I don't think.

If not would it be possible to revert it?

Thanks

Jakub

Hi,

I was wondering why stream API has been changed in this commit:

https://github.com/php/php-src/commit/92d27ccb0574f901a107409a7fec92888fa2b82f

Basically all char pointers have been constified. The thing is that this
commit leads to compilation warning for many extensions.

In my case I use php_stream_locate_url_wrapper and want to compile my
extension (fann) without any warnings. Because this change is in master and
not in 5.5-, I will have to add some ifdefs and cast it for 5.6+.

The thing is that php_stream_locate_url_wrapper and other stream fuctions
are often used for function parameters from zend_parse_parameters which are
just char *. Then the values have to be casted.

Your claim actually makes not much sense... if you've got chat* and
you pass it to a function accepting "const char*" there should'nt be a
warning at all.

But I guess you actually declrared your variable "const char*" and get
the warning from zpp? That is not necessary:

void const_dummy(const char *data) {
printf("got %s\n", data);
}

PHP_FUNCTION(dummy)
{
char *data;
if (SUCCESS != zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &data)) {
return;
}
const_dummy(data);
}

FFI: http://en.wikipedia.org/wiki/Const#Constant_function_parameters

--
Regards,
Mike

Le 02/10/2013 20:41, Jakub Zelenka a écrit :

Hi,

I was wondering why stream API has been changed in this commit:

https://github.com/php/php-src/commit/92d27ccb0574f901a107409a7fec92888fa2b82f

Basically all char pointers have been constified. The thing is that this
commit leads to compilation warning for many extensions.

In my case I use php_stream_locate_url_wrapper and want to compile my
extension (fann) without any warnings.

Sorry to have not read enough carefully your mail.

-PHPAPI php_stream_wrapper *php_stream_locate_url_wrapper
(const char *path, char **path_for_open, int options TSRMLS_DC);

+PHPAPI php_stream_wrapper *php_stream_locate_url_wrapper
(const char *path, const char **path_for_open, int options TSRMLS_DC);

While is absolutly safe (and often useful) to switch from (char *) to
(const char *), it is not with (char **)

No compatibility in ever way.
... expected ‘const char **’ but argument is of type ‘char **’ ...
... expected ‘char **’ but argument is of type ‘const char **’ ...

So I agree, this one could probably be reverted.

As I found awful having to see more code like this (ex from twig)

#if PHP_API_VERSION >= 20100412
zend_get_object_classname(object, (const char **) &class_name,
&class_name_len TSRMLS_CC);
#else
zend_get_object_classname(object, &class_name, &class_name_len
TSRMLS_CC);
#endif
return class_name;
}

Remi.