Re: cvs: pecl /ibm_db2 TODO config.m4 ibm_db2.c php_ibm_db2.h

Hi, Dan Scott wrote:

dbs Tue Apr 5 20:17:49 2005 EDT

  Added files:                     /pecl/ibm_db2	ibm_db2.c config.m4 php_ibm_db2.h TODO   Log:

First cut of ibm_db2 extension for IBM DB2/Cloudscape and Apache Derby. ------------------------------------------------------------------------

It's great that there is now a DB2 extension! Some comments on the first cut: 1) Looks like currently you don't use RINIT/RSHUTDOWN. If you don't plan to use them then please remove them as functions and in zend_module_entry ibm_db2_module_entry 2) In MINIT you register 2 resource types, namely : - conn struct - stmt struct Because these are the visual names when someone dumps a variable they should be more descriptive, like "DB2 Connection" and "DB2 Statement". The best is to make a preprocessort #define in the beginning of the file and then use this define every time it is needed (for example where you fetch a resource). 3) Please use only tabs. Most of the file uses tabs but in some places spaces are used. Open the file in a decent editor, set the tab to 8 spaces and scroll, you will see the problematic snippets of code. 4) In int _php_db2_bind_column_helper(stmt_handle *stmt_res) add one more tab in front of the "break" statements 5) In _php_clear_stmt_err_cache() instead of fetching the TSRM data with the macro, pass the data as a parameter _php_clear_stmt_err_cache(TSRMLS_D){ ... ... } When you use it _php_clear_stmt_err_cache(TSRMLS_C); 6) In param_node* build_list( stmt_handle *stmt_res, int par ....) Please don't put the "else" on the next line after the closing bracket. This is not according to the CODING_STANDARDS (a file that exists in CVS). As I see the else on the next line appears also in other places. It would be nice if you fix them too :) 7)In PHP_FUNCTION(db2_execute) After fetching the resource there is a check, namely if (stmt). Please change it to : if (!stmt) { return; // or RETURN_FALSE; } This will make the rest of the code to shift left (easier for reading). As far as I see the same applies to most of the places a resource is fetched. Also this won't return NULL in case stmt is C NULL. In other functions you return FALSE in this case. 8) Don't add \n at the end of the error messages thrown with the help of php_error_docref() 9) Around PHP_FUNCTION(db2_foreignkeys) please add a pair of new lines. The same applies to PHP_FUNCTION(db2_primarykeys), PHP_FUNCTION(db2_procedurecolumns), PHP_FUNCTION(db2_specialcolumns), PHP_FUNCTION(db2_tableprivileges). 10) In PHP_FUNCTION(db2_field_width) There are few places where you have one more tab (inside if() {} blocks), 11) db2_result_all(resource stmt) - "Returns an HTML table containing the remaining rows of a result set " Is that needed? Finally, thans one more time for contributing! Andrey