Bonsoir,

On Do, 2017-05-11 at 14:25 +0200, simon .barotte wrote:
> code in order to advise me, or to guide me on some point to improve .
> 
> The code is available here : gpio_extension
> <https://github.com/simon88/gpio_extension>

I had a *quick* look so this is no proper review, but a few comments:
Please check your indention, there seem to be tabs and spaces mixed
(PHP typically uses tabs)

I see you've added the GPLv3 as LICENSE file, while all files state
it's licensed under PHP License 3.01. Mind that PHP License and GPL are
incompatible.

The le_gpio_extension declaration can be removed. That's added by
ext_skel and could be used for registering a resource (which shouldn't
be done anymore ... probably one could patch ext_skel to get rid of
this)

stdbool.h is C99. For a stand-alone extension this is fine, for core
PHP we however didn't agree to use C99, yet (unless I missed the
switch, which could happen, I don't follow closely)

PHP_METHOD(Gpio, __construct)  seems to abuse return_value.

In general it is also bad to change the return_value before
callingzend_parse_parameters ... but here we're in a constructor so it
shouldn't matter much.

You shouldn't use zend_error, that's for the engine. Use
php_error_docref. Also don't use E_ERROR. E_ERROR is for critical
situations where the engine can hardly continue running. Probably you
want a custom exception.

In your MINIT you allocate
   zval *cd = (zval*)malloc(sizeof(zval));
but never use it. Related: Many people discourage casting malloc's
return value http://stackoverflow.com/a/605858/206302

You mix long and int types. If you have a 32bit system that is no
problem, but as soon as you use a 64 bit system this leads to issues.
i.e. this:
   static int readPin(int pin);
   zend_long pin;
   result = readPin(pin);

Another comment about readPin: If reading failes you don't close the
file handle.

You might also be interested in those common comments I include in such
reviews: https://wiki.php.net/internals/review_comments

And as a final thought: I think this is a great training thing! However
I don't see anything which *Requires* doing this in an extension. Doing
this in pure PHP should work with neglectable overhead.

johannes


-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Reply via email to