Dead Code in ogr_fromepsg.cpp

[Kyle Shannon]

The return value at:

​https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogr_fromepsg.cpp#L228

is never NULL. CVSGetField() returns an empty string, never NULL:

​https://trac.osgeo.org/gdal/browser/trunk/gdal/port/cpl_csv.cpp#L980

so the else clause of

if( pszUOMName != NULL ){...} 
else {...}

has never been executed, and never tested:

​http://rawgit.com/rouault/gdalautotest-coverage-results/master/coverage_html/ogr/ogr_fromepsg.cpp.gcov.html

(line 266).

Possible fix:

Index: ogr/ogr_fromepsg.cpp
===================================================================
--- ogr/ogr_fromepsg.cpp	(revision 30295)
+++ ogr/ogr_fromepsg.cpp	(working copy)
@@ -228,7 +228,7 @@
     const char *pszUOMName = CSVGetField( pszFilename,
                               "UOM_CODE", szSearchKey, CC_Integer,
                               "UNIT_OF_MEAS_NAME" );
-
+    CPLAssert( pszUOMName );
 /* -------------------------------------------------------------------- */
 /*      If the file is found, read from there.  Note that FactorC is    */
 /*      an empty field for any of the DMS style formats, and in this    */
@@ -237,7 +237,7 @@
 /* -------------------------------------------------------------------- */
     double dfInDegrees = 1.0;
 
-    if( pszUOMName != NULL )
+    if( !EQUAL( pszUOMName, "" ) )
     {
         const double dfFactorB =
             CPLAtof(CSVGetField( pszFilename,
@@ -304,14 +304,7 @@
 /*      Return to caller.                                               */
 /* -------------------------------------------------------------------- */
     if( ppszUOMName != NULL )
-    {
-        // CPLStrdup returns a copy of "" if given a nullptr.
-        if( pszUOMName != NULL )
-            *ppszUOMName = CPLStrdup( pszUOMName );
-        else
-            *ppszUOMName = NULL;
-    }
-
+        *ppszUOMName = CPLStrdup( pszUOMName );
     if( pdfInDegrees != NULL )
         *pdfInDegrees = dfInDegrees;

goatbar has been working in this area while fixing coverity, probably best for him to look it over(CID:1074894).

It was difficult to create a test test, as the csv file would have to inaccessible for EPSGGetUOMAngleInfo(), but accessible by the caller OGRSpatialReference::importFromEPSGA();

[Kurt Schwehr]

I'm looking at it. I still haven't proven that CVSGetField() never returns a nullptr.

Work includes r30388 r30389 r30392 trying to see the flow a little easier. This has to always be not a nullptr:

  return( papszRecord[iTargetField] ); 

I'm pretty sure that is never a nullptr, but it's hard to prove.

[Even Rouault]

trunk r30755 "CSVGetField(): make it (more) obvious that the function cannot return nullptr (#6113, CID 1074894)"

trunk r30756 "Remove useless comparison to nullptr since CSVGetField() cannot return nullptr (patch by KyleS, #6113, CID 1074894)"

If Coverity still complains, it is just too stupid ;-) It should already have understood the semantics of CSLCount()