[gdal-dev] RFC 39

[Even Rouault]

> What would it take to adopt this RFC?

Ari,

I'm pretty satisfied with the current state of the RFC. From a recent 
discussion with Frank, I think he has perhaps some comments to add ?

I've looked at the latest version of the patch. A few comments :

* You should not use GetFeaturesRead() to know how many features have been 
iterated to compute the progress rate. It is not a reliable way of knowing it, 
since it is up to the driver to do the proper incrementation (and many drivers 
don't : this counter is usually only used for debugging purposes). But the 
more fundamental reason is that this counter is global to the session. So if 
the user has already iterated over the input layer before calling the new 
functions, the counter will not be 0. Also, some implementations of 
GetFeatureCount() (actually the default non specialized implementation of 
GetFeatureCount()) might also cause the counter to be incremented. So I'd 
suggest maintaining your own local counter instead.

* At the end of the loop, it would also be appropriate to call the progress 
callback with a progress rate of 1.0. That will make, for example, the output 
of the GDALTermProgress implementation to be nicer.

* In set_filter_from(), pLayer->SetSpatialFilter(geom-
>Intersection(pGeometryExistingFilter)) likely leaks the passed geometry. You 
should assign it to a temporary variable and delete it afterwards.

* In the code block triggered if the pfnProgress returns FALSE, there should 
be a delete x;

* In the Doxygen, it would be appropriate to mention that these routines use 
geometric operations that are only available if GDAL is compiled with GEOS 
support.


I've also read your note in the RFC about performance. It is interesting to 
see that envelope computation is the limiting factor when the method layer is 
in memory. It is an interesting track for a later optimization where envelope 
would be cached instead of being computed each time. Spatial indexing would 
also do the trick. It reminds me that a few years ago I've added a 
cpl_quad_tree.cpp implementation that is a direct transposition of the  
shapelib implementation, but not tied with shapefiles. It has never been 
seriously used AFAIR, but it could perhaps be a starting point to do in-memory 
spatial indexing.

Even