OGR Support for Selafin format

[fhissel]

Selafin is a format used by some finite element software (mostly ​Telemac hydraulic model) to store numerical values at different points or elements and for different time steps.

This driver allows read-write support for Selafin format in OGR.

[fhissel]

Test case 1

[fhissel]

Test case 2

[Even Rouault]

A few remarks from my code review :

• OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string
• The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.
• port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping
• the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.
• the CreateLayer() and DeleteLayer() should be completely removed, or disabled with #ifdef notdef if you intend implementing them one day.
• Same for CreateDataSource(), DeleteDataSource().
• there are very long lines with if else clause on the same line, which make them difficult to read
• the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Questions :

• did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?
• did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant
• did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Wishes :

• could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

[fhissel]

Replying to rouault:

Thanks for your code review! I'm going to fix the driver.

A few remarks from my code review :

• OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

• The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The ​standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

• port/cpl_port.h has macros to detect the endianness at compile time and do byte-swapping

OK, I will use those macros.

• the use of CE_Fatal is strongly discouraged. It causes an abort() which is not good for a library like GDAL that might be integrated in GUI applications like QGIS etc... CE_Failure with proper return from methods is much prefered.

OK, I will change that

• the CreateLayer() and DeleteLayer() should be completely removed, or disabled with #ifdef notdef if you intend implementing them one day.
• Same for CreateDataSource(), DeleteDataSource().

OK

• there are very long lines with if else clause on the same line, which make them difficult to read

OK

• the sample files are a bit big to be integrated in the autotest suite. Would it be possible to have files < 20 KB ?

Yes, I will attach new smaller files.

Questions :

• did you compile the apps/test_ogrsf utility (cd apps; make test_ogrsf or nmake /f makefile.vc test_ogrsf.exe) and run it on sample files ?

Yes, all tests passed without error (at least the first 100 or so, until I stopped the program).

• did you test that the compilation was warning free with gcc and MSVC ? You can for example use the Vagrant environment for Linux gcc and MSVC 2008 Express (under wine). See http://trac.osgeo.org/gdal/wiki/Vagrant

It was OK with gcc under Linux, but I did not test is with MSVC. I will try it.

• did you test ogrinfo/ogr2ogr/test_ogrsf under a memory debugger like Valgrind ?

Yes, valgrind --leak-check=full did not detect any leak.

Wishes :

• could you write a Python script to be integrated in autotest/ogr that would test the driver with sample files ?

OK, I am going to add one.

[Even Rouault]

Replying to fhissel:

Replying to rouault:

• OGRSelafinDataSource::Open() will read pszFilename[-1] if passed an empty string

The second line in the function (if (*pszFilename==0) return FALSE) was meant to avoid that, but I'm going to check it more carefully.

Indeed. I missed that line. So that part is OK.

• The use of long (whose size may be 32 or 64 bit long accross platforms) in read_integer() / write_integer() would be better replaced by int which is 32bit on all current platforms I'm aware of.

The ​standard mentions 16-bits int although it seems quite rare, but that would be too few for the usual number of features (up to 32767...). Anyway, I think the implementation of the driver for integers does not depend on their actual size.

Yes, I know. I'm generally suspicious about long use - I've seen many times wrong uses like fread(&a_long, 1, 4, f) -, but in your code, it looks OK.

[fhissel]

Test case

[fhissel]

Replying to rouault:

Replying to fhissel:

Replying to rouault:

I've just uploaded a new version of the driver. This one supports writing to Selafin format. A smaller test case and an autotest script are attached.

The driver has been tested with valgrind and test_ogrsf (there is one error in test_ogrsf but it is caused by a normal renumbering of FIDs when inserting or deleting a feature in some kind of layers).

I have not tried to compile it with MSVC (no Windows available).