On Feb 4, 2010, at 5:53 PM, Andreas Köhler wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi John, > > I like code to be improved, but a few things come to my mind. You can > probably fix them rather easily. > > - - Watch out for memory leaks. E.g. you must free the result of > gnc_path_get_pkgdatadir() after usage, otherwise you leak it. The > function documentation typically explains the memory management of > parameters and return values. > - - Try to use glib functionality to avoid mistakes and improve > portability. E.g. if you do not explicitly pass NULL to getcwd(2) then > you are expected to provide a char[] buffer for the function to fill in. > g_get_current_dir() is the solution and devhelp your friend :) > - - BEGIN and LEAVE should wrap all possible paths through a function (if > needed at all). I.e., start with BEGIN and add LEAVE before every return > or the end of the function. > - - I am picky wrt trailing whitespaces, no tab characters unless already > in the surrounding code, printfs (like g_printerr), unneeded code newly > commented out > > I am more interested whether the removed functionality really is not > needed anymore or fully replaced. Is .gnucash created in time, do we > need to support file: or xml: schemas anywhere? > > Personally, I could not guess about the patch on Windows, someone (not > me ;-)) would need to give it a try. >
Thanks for the reminder about freeing g_strings, and I agree that g_get_current_dir() is a better choice than getcwd(). It's ENTER and LEAVE, I think, and your point is well taken (and fixed). That diagnostic g_printerr() got left in by mistake. Thanks for pointing it out. I'm not terribly concerned about trailing whitespace in C, it's not significant -- but I ran delete-trailing-whitespace on the buffer anyway. It will add some bogus lines to the diff, but is otherwise harmless. ~/.gnucash (or whatever $GNC_DOT_DIR is, which is the point of the changes) is handled by gnc_build_dotgnucash_dir(). The deleted MakeHomeDir() duplicated gnc_build_dotgnucash_dir()'s functionality, but did so with hard-coded paths. The URI scheme detection code (for file: and xml:) was already handled in the companion function in the file, xaccResolveURL(). xaccResolveFileName() is used twice directly, and in both cases if a URI is provided instead of a filename, other parts of the calling function will fail. This patch still passes src/engine/test/test-resolve-file-path, which looks like the applicable unit test. The new patch is attached below. I'm still looking for some feedback from an M$Win build before I commit. Regards, John Ralls
dot_gnucash.patch
Description: Binary data
_______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel