Re: Fwd: PHP7 extension to control raspberry GPIO

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