Hi Ari, > I've uploaded a new patch, which hopefully corrects the issues Even > brought up. Below are comments.
I've reviewed quickly and the new version seems to address my comments well. > > I've not added the Append or Buffer methods, which basically work on > just one layer. I believe such functionality is simple enough to create > using feature iteration. > > Ari > > the patch: > http://trac.osgeo.org/gdal/attachment/wiki/rfc39_ogr_layer_algebra/rfc39.patch > Hum, I'm surprised that it has survived your testing ;-) : GetSpatialFilter() might return NULL (in fact, in the general case, it will), so GetSpatialFilter()->clone() should segfault. CPLMalloc() cannot return NULL. Internally, it will abort if the memory allocation failed. So you can just remove the if (!mapInput) // if (!mapMethod) tests afterwards, or use VSIMalloc() instead. For consistency, memory allocated with CPLMalloc()/VSIMalloc() should be freed with CPLFree()/VSIFree(), and not free(). (There's an alternate memory allocator that does consistency checking that can be enabled by defining #define DEBUG_VSIMALLOC in port/cpl_vsisimple.cpp) For the final implementation, the new methods that are the object of the RFC will need Doxygen doc. I don't see a strong reason to publish OGR_F_SetFieldsFromWithMap() in the C API. This is mostly usefull for internal purposes for now. About SetFieldsFrom(), it also changes SetFID() and SetStyleString(). This is a bit counter-intuitive. It will be indeed interesting to make SetFeatureFrom() call SetFeatureFrom() to avoid code duplication. As far as the RFC is concerned, do you intend writing tests in the autotest suite for the new methods ? Best regards, Even _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
