[PHP-INTERNALS] 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.

There's not really much win. In most cases the generated code is exactly
the same.*) Such changes are nice to spot bugs as the compiler has more
knowledge of the intention and to make C++ compilers happy when using
these APIs as C++ is stricter on const.

johannes

*) there are few cases where this helps with inlining or passing params
via register, but here we mostly deal with exported APIs which have to
follow the platforms ABI rules.

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.

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

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.

Yeah, this is just for (char **) -> (const char **)

I found this in php_stream_locate_url_wrapper. I think that this should be
changed to

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

There is a similar change for php_glob_stream_path_split but it doesn't
have any influence on extensions as it is a static function.

Sorry about the zpp comment. It really didn't make any sense! :)

Jakub