Re: New Extension: PHP-SVM

Hi, some minor comments on the code: svm.c:24-26 zend_class_entry *php_svm_sc_entry; zend_class_entry *php_svm_model_sc_entry; zend_class_entry *php_svm_exception_sc_entry; These could be made static. svm.c:43 #define SVM_SET_ERROR_MSG(intern, ...) snprintf(intern->last_error, 512, __VA_ARGS__); This is C99, not all compilers support C99. If you want the code to work on all compilers either make this a function or always write the full line of code. svm.c:70 void print_null(const char *s) {} This could be made static. svm.c:72-109 (and other functions) static zend_bool php_svm_set_double_attribute(php_svm_object *intern, SvmDoubleAttribute name, double value) It's nicer to use TRUE/FALSE instead of 1/0. Especially as one is often confused as SUCCESS/FAILURE in PHP are the other way round ... svm.c:200-201 (and other places) if (!value) break; our coding standard uses { } always, even for a single line. svm.c:486 (same for SvmModel::__construct) if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "") == FAILURE) { You could use zend_parse_parameters_none() there to make it shorter (PHP 5.3). More relevant: You should throw an exception on failure. Passing wrong arguments will result in an invalid object right now. svm.c:885 x = emalloc((array_count + 1) *sizeof(struct svm_node)); use safe_emalloc to prevent overflows. svm.c:1093-1094 //extern void (*svm_print_string) (const char *); no C++ / C99 comments but /* */ (or remove code commented out) php_svm.h:36 zend_bool test; This doesn't seem to be needed, like the whole globals declarations. php_svm.h in general: I suggest keeping the php_$extname.h header small. For a static build it is included from main/builtin_functions.c for having access to phpext_ $ext_ptr, the less things defined there, the less potential issues. Final remarks: These all are minor things, overall it looks good. Please mind that I didn't do a full review, so I didn't went through all buffers and checked whether they are being accessed with the proper offsets etc. johannes On Sat, 2011-04-23 at 12:20 +0100, Ian Barber wrote: > Hi all, > > There was some discussion a while back about a libsvm (Support Vector > Machine, a machine learning/classification library) extension. I would like > to add an implementation I have to PECL, if it looks all right to people: > > https://github.com/ianbarber/php-svm > > The underlying library is available in package management for a few distros, > and is under the modified BSD license and is widely used with bindings in > many languages. Unfortunately the site for the library seems to be down this > weekend (http://www.csie.ntu.edu.tw/~cjlin/libsvm/). > > Would appreciate any feedback! > > Thanks, > > Ian