Hi, On Fri, 2014-09-05 at 02:40 -0700, Boro Sitnikovski wrote: > I've made a PHP extension for managing (e.g. playing, decoding, > retrieving metadata, mixing) module tracked files > (http://en.wikipedia.org/wiki/Module_file), which is using the libxmp > library. > You can find the code for the extension here: > https://github.com/bor0/xmp
From a quick glance over the code it looks good. Some minor comments:
* In one place I noticed you used a size_t for a string lenth for
"s" modifier of zend_parse_parameters. This has to be int (in
PHP 5.x)
* You should use zend_parse_parameters_none() in function taking
no arguments for being consistent with other extensions
* In xmp_create_context error handling looks wrong
* Using add_assoc_*_ex instead of add_assoc_* lets you provide the
length of the key, this saves a strlen() call for each of those
* If you move the xmp_functions list to the end of the file you
can remove the declarations from the header. I'd prefer that
stylistically, see also
https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal
* Two comments on licensing: libxmp licensing is confusing. The
sourcefore page lists LGPL 2 and GPL 2. Looking at the source I
see a mixture of files with LGPL, BSDish, public domain, no
license. This is no problem for PHP (i.e. no GPL) but if you
talk to them you might as them to clean that up. Secondly you
are using PHP license. Debian sometimes claims this can only be
used for PHP itself, we disagree. Some people therefore prefer
BSD or Apache license.
These are all minor things. If nobody else sees an issue feel free to
request PECL account for publishing on pecl.php.net via
http://pecl.php.net/account-request.php . If you want to host the
primary git repo on git.php.net (will be mirrored to github) and bug
tracker and out git setup make sure to set the checkbox (unless you
already have an account) and tell us so we can enable that.
johannes
signature.asc
Description: This is a digitally signed message part
