Hi,
On Fri, 2013-11-01 at 07:19 -0700, Greg Oschwald wrote:
> Currently the extension is distributed along side our pure PHP reader:
>
> https://github.com/maxmind/MaxMind-DB-Reader-php (in the ext directory)
>
> We would love any feedback you have.
>
> Thank you for your consideration,
Overall: +1.
A few minor comments, some more for other reviewers who coulddisagree
with me ;-)
* Your license seems to be LGPL2.1. This is fine but we prefer PHP
License 3.01 or BSD for extensions.
* You should add license headers to the source files to make this
more explicit
* You don't follow PHP coding style, but seem consistent so this
is fine. Exception usage isn't like we'd do it, but your aim
seems to be to be compatible with the PHP implementation so this
is fine
* We try to be C89 compatible which requires declaring variables
first on top of a block see
https://wiki.php.net/internals/review_comments#don_t_use_c99_for_portability_reasons
* Your manipulation of CFLAGS in config.m4 prevents from
compilation with other compilers, either detect whether the
compiler supports these or let the user set the while running
configure.
* The config.m4 links against an external library, as a guideline
in such a case we use "--with-foo" not "--enable-foo" as option.
By using "--with-foo" the user could pass the location of the
library in case it was installed in a non-standard path
* the config.w32 file doesn't link that library at all, either it
should be made working (Pierre or Anatol probably might help?)
or removed
* Your php-maxminddb header includes quite a few other headers,
this is discouraged, see
https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal
* maxminddb_ce->ce_flags |= ZEND_ACC_FINAL | ZEND_ACC_ABSTRACT;
this seems weird, final and abstract!?
As said all nothing major.
johannes
--
PECL development discussion Mailing List (http://pecl.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
