Re: php_ssdeep Fuzzy Hashing Extension

On Tue, 2010-09-07 at 11:16 +0100, Simon Holywell wrote: > Hi, > > I have recently written my first extension to make use of the API > exposed by the GPLv2 licensed ssdeep project > (http://ssdeep.sourceforge.net/). My code is already published > http://www.ohloh.net/p/php_ssdeep with the Git repo on GitHub: > http://github.com/treffynnon/php_ssdeep Having the licenses cleared (I assume at least :) ) I had a look at the code, some small comments, nothing critical: 1) proto lines Please add these "proto" lines we have in front of all PHP_FUNCTIONS, they help to see the correct parameters using a simple grep and give an idea once the functions become more complex. 2) Add arg info Adding ZEND_ARG_INFO blocks would begood so parameter info can be retrieved using reflection. This is really helpful for getting some documentation done: http://wiki.php.net/doc/scratchpad/pecldocs 3) Don't indent CPP macros The # for the C Pre processor have to be in the first column of the line according to the specs. There might be whitespaces behind them though. #ifdef FOO # define BAR 1 #endif is valid C. #ifdef FOO #define BAR 1 #endif not. (I general indention due to the guarding declartion isn't really suggested either) 4) C99/C++ comments C90, what we ar eusing does not allow // as comments, always use /* */ or #ifedf code out. (seen in php_ssdeep.h) 5) php_ssdeep.h should only include what is needed In case of an static build all php_$extname.h headers are included from main/builtin_functions.h if there are too many includes it becomes likely there are conflicts between declarations in the header. Move #include <fuzzy.h> to the .c file if possible. 6) config.m4 should use "with" This extension links against some library, the config.m4 file should therefore specify a --with-ssdeep option instead of enable,additionally the config.m4 file should check for the location of the lib (see PHP_CHECK_LIBRARY in http://php.net/buildsys.configunix ) 7) Does fuzzy_hash_buf return the length? I didn't check the library you are using, does fuzzy_hash_buf() return the length so RETURN_STIRNGL() can be used? Or does it signal errors in some way? 8) Tests Would be great to add some .phpt tests, see http://qa.php.net/write-test.php 8) hint on the build system Your build.sh script copies the source files, this is not needed. You can build PHP and the extensions outside the source tree: $ cd src $ phpize $ cd .. $ mkdir build $ cd ../build $ ../src/configure --with-foo $ make $ make test These are all minor things, in general I think this is ok for PECL. johannes