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 _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
