Opened 10 years ago
Last modified 10 years ago
#5442 closed enhancement
OGR Support for Selafin format — at Version 4
Reported by: | fhissel | Owned by: | fhissel |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | OGR_SF | Version: | unspecified |
Severity: | normal | Keywords: | selafin driver ogr |
Cc: |
Description (last modified by )
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.
Change History (7)
by , 10 years ago
Attachment: | f2d_bowl_fe.slf added |
---|
follow-up: 2 comment:1 by , 10 years ago
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 ?
follow-up: 3 comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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.
follow-up: 4 comment:3 by , 10 years ago
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.
comment:4 by , 10 years ago
Description: | modified (diff) |
---|
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).
Test case 1