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

On Apr 6, 2005 3:28 AM, Andrey Hristov <php@hristov.com> wrote: > 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! We're pretty excited about it -- obviously we still have some work to do (looking in the TODO file) but it seemed like the right time to get code out there for the keen eyes of the PECL folk to look over. And good thing, too :) Would you believe I spent about an hour walking through the code with Vim last night trying to make it all comply to the CODING_STANDARDS before committing the file? I poked around on #php.pecl looking for code formatting tool suggestions, but didn't have any luck, and indent, Vim's auto-indent, and kdevelop's source reformatter tended to go strangely awry in places so I took a run at it with plain old Vim (with some success, but clearly not enough). Any tooling suggestions for the future would be greatly appreciated :) > 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 Right-o. If they're still not needed after persistent connections are implemented, we'll remove them as you suggest. > 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). Will do -- thanks! Probably a few commits down the road though. > 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. Solidly my fault. I actually used vim, but obviously messed this up. Should be fixed quickly. > 4) In int _php_db2_bind_column_helper(stmt_handle *stmt_res) add one more > tab in front of the "break" statements Argh. I shouldn't have missed those. Fixed > 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); Interesting -- I assume the same would apply to _php_clear_conn_err_cache() as well. Thanks for the tip! We'll make the change, run our tests, and commit it if all goes well (as I'm sure it will). > 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 :) Yep, those are easy fixes. Will do. > 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. Very good point. I'll ensure that we fix things up accordingly. > 8) Don't add \n at the end of the error messages thrown with the help of > php_error_docref() Right, makes perfect sense. We'll make the fix and adjust our tests accordingly. > 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). Will do. > 10) In PHP_FUNCTION(db2_field_width) > There are few places where you have one more tab (inside if() {} blocks), Right! Fixed. > 11) db2_result_all(resource stmt) - "Returns an HTML table containing the remaining > rows of a result set " > > Is that needed? Actually, no -- it was left over from the earliest days of the extension. I'll pull it; we don't really want to support people using that function :) > > Finally, thans one more time for contributing! Andrey, thank you very much for the quick review -- that will certainly get us on the right track, as we clearly have some more commits in the pipeline, and it will definitely help others reading our code. Thanks, Dan