FAQ
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

Search Discussions

  • William Bartlett at Oct 3, 2013 at 3:01 am
    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

    On Wed, Oct 2, 2013 at 2:41 PM, Jakub Zelenka wrote:

    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
  • Johannes Schlüter at Oct 3, 2013 at 4:17 pm

    On Wed, 2013-10-02 at 23:01 -0400, William Bartlett wrote:
    My understanding is that the const triggers some optimizations because the
    compiler better understands how the variables are used.
    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.
  • Remi Collet at Oct 3, 2013 at 3:54 am

    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
  • Michael Wallner at Oct 3, 2013 at 7:55 am

    On 2 October 2013 20:41, Jakub Zelenka wrote:
    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
  • Remi Collet at Oct 3, 2013 at 8:59 am

    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.
  • Jakub Zelenka at Oct 3, 2013 at 9:27 am

    -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;
    }
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedOct 2, '13 at 6:41p
activeOct 3, '13 at 4:17p
posts7
users5
websitephp.net

People

Translate

site design / logo © 2022 Grokbase