Re: Adding libmaxminddb extension to PECL

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