[gdal-dev] RFC 48: Geographical networks support

[Even Rouault]

Le lundi 08 septembre 2014 14:08:11, Mikhail Gusev a écrit :
> Hello all.
> My work on GSoC 2014 project is finished and I made an RFC in order to
> discuss the integration of my subproject into GDAL:
> http://trac.osgeo.org/gdal/wiki/rfc48_geographical_networks_support. I
> think it may be necessary to make some small changes/improvements before
> the integration will be possible and I'm ready to make them. Anyway I'll be
> glad to hear your thoughts and ideas about my project and its integration.

Mikhail,

Not much to say on the RFC itself. Perhaps you should mention who will take 
care of committing in the source : Dmitry I guess ?

I looked at the code changes:
https://github.com/MikhanGusev/gdal/compare/OSGeo:trunk...trunk

My remarks:
- any reason not to licence  autotest/gnm/gnm_gdalnetwork_test.py
 under X/MIT ? There are a few files LGPL licenced in autotest, but this is 
more historical remain than intended policy.
-  gdal/apps/GNUmakefile : in clean target, analysis/*.o is probably unneeded
- which gnm headers are installed ? gnm.h gnm_frmts.h gnm_api.h 
gnmstdanalysis.h ?
- header files should have guards against multiple inclusion. At least seen for 
gnm/analysis/gnmstdanalysis.h
- in implementations there are many maps, vectors, etc., so I guess some 
algorithms may consume a lot of memory if operating on huge networks. The 
documentation should make it clear.
- GNMGdalStdRoutingAnalyser and GNMGdalStdCommutationsAnalyser: currently have 
only one method and no member variables. Do you foresee future extensions to 
add member variables ? If not, they might go to GNMGdalStdAnalyser, no ? And 
what about non-GDAL network ? Shouldn't the interface of 
DijkstraShortestPath() and ResourceDestribution() go to GNMStdAnalyser ? Open 
questions : just wondering of what it might look like if other network 
implementations are done. I can imagine that the dijkstra alg would have 
similar interface.
- gnm/frmts/gnm_frmts.h :  I'm a bit concerned about exposing (installed 
header + CPL_DLL) an interface that has not yet been implemented. My intuition 
is that it might change once the first one or two implementations have been 
made. So maybe keep it internal/experimental for now.
- gnm/frmts/gnm_frmts.h : "static const char *GNMAllFormats[] = {" should not 
be exposed in a .h file
- gnm/frmts/gnm_frmts.h :  static std::vector<std::string> _regNetFrmts : same 
remark as above
- gnm/frmts/gnm_frmts.h :  GetFormatByName() and RegisterAllNetworkFormats() 
should not be in a .h and lack "namespace"
- gnm/gnm.h : static const char *GNMGdalSupportedDrivers[] should be moved 
somewhere else
- gnm/gnm.h : most #define are not of public use. might be moved in a 
gnm_priv.h
- gnm/gnm.h : typedef int GNMDirection and the 3 following defines. A typedef 
enum would seem more appropriate. Same for GNMErr (although I can see this 
pattern in ogr_core.h. not clear why enum have not been chosen)
- gnm/gnm.h : GNMManager::Close() the GNMNetwork should know if it is native 
or not. The user shouldn't have to specify it again.
- gnm/gnm.h : GNMManager::GdalCloseNetwork(). Ideally this method should not 
exist and Close() should work for all kind of network objects.
- gnm/gnm.h : GNMNetwork class definition. Commented methods that could be 
removed. TODO that no longer apply
- gnm/gnm.h : IsDriverSupported() --> IsDatasetDriverSupported() for 
consistency between name and argument ?
- gnm/gnm.h : std::pair<char**,int> GetClassLayers () : very unusual interface 
in GDAL. I'd say return a char** NULL terminated for consistency to other 
methods in the same class
- gnm/gnm_api.h: all functions are prefixed by GNMGdal. But some methods 
actually belong to the base GNMNetwork class, ant not specifically to 
GNMGdalNetwork. For example CreateLayer(), CreateFeature(), and many others
- gnm/gnm_tut.dox : #include "analysis/gnmstdanalysis.h" . Is the analysis 
directory valid for an installed GDAL ?
- gnm/gnmgdalnetwork.cpp : _makeNewLayerName(): newName might overflow. instead 
of 100, use strlen(name) + 50. Or return a std::string to avoid manual dealing 
with memory
- gnm/gnmgdalnetwork.cpp : GNMGdalNetwork::CreateLayer() : the call to  
_makeNewLayerName return a char* not a const char*.
- GNMGdalNetwork::IsDriverSupported (): CSLFindString() >= 0 could be used to 
replace the loop
- GNMGdalNetwork::GetSupportedDrivers () : return 
CSLDuplicate(GNMGdalSupportedDrivers)
- GNMGdalNetwork::GetMetaParamValueName () : return a "const char*" that must 
be freed by delete[]. Quite unusual. Better return a "char*". Other issue, it 
is allocated with new char[], but the method is bound to C. C callers cannot 
call delete[]. So you have to allocate with CPLMalloc(), and ask the caller to 
free with CPLFree(). Isn't there a risk of overflow with 254 characters ?
- GNMGdalNetwork::GetMetaParamValueDescr () : same remarks as above
- GNMManager::CreateConnectivity () : I'm confused by the 'native' term. In 
this method, native=false seems to imply the GDAL "proprietary" network format 
that can work with any vector driver that has similar capabilities than 
shapefile. Whereas in the RFC text, it seems to imply the contrary ( "network 
of ”GDAL-native” format").
-  GNMNetwork::AutoConnect() :" if (lineGFID == 684) { int x = 9; } " debug 
left-over ?

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com