Opened 13 years ago

Closed 8 years ago

Last modified 7 years ago

#3178 closed enhancement (fixed)

GDALDestroyWarpOptions to do nothing for null

Reported by: Mateusz Łoskot Owned by: warmerdam
Priority: normal Milestone: 2.0.0
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords:
Cc:

Description (last modified by Mateusz Łoskot)

GDALDestroyWarpOptions function should follow behaviour of std::free function and other standard C/C++ deallocators and should do nothing if nullptr is passed.

Currently, it throws error if psOptions is nullptr.

The change is trivial:

void CPL_STDCALL GDALDestroyWarpOptions( GDALWarpOptions *psOptions )
{
   if (psOptions)
   {
      // current definition
   }
}

Actually, this should apply to all GDAL/OGR deallocators (Destroy and Close functions).

Change History (9)

comment:1 by Mateusz Łoskot, 13 years ago

Description: modified (diff)

comment:2 by Jukka Rahkonen, 8 years ago

If change is trivial and as result something good happens without drawbacks, then perhaps mloskot could just do the change?

comment:3 by Mateusz Łoskot, 8 years ago

That is not an issue, I can provide the patch or commit it directly to the upstream repo. This issue has been opened as a request for comment and confirmation if I shall proceed or not.

Most of the tickets I opened as request for comments and which haven't been closed yet are waiting for final word from Frank or Even. So, please, do not close them w/o Frank's or Even's decision. Thanks.

comment:4 by Even Rouault, 8 years ago

Mateusz, looks go to me. Please go ahead with the change

comment:5 by Mateusz Łoskot, 8 years ago

Even,

I have reviewed current destroy/free-like functions and many of them either already include proposed changes and are no-op on nullptr input, or perform input pointer validation with the common macro. Some functions report CE_Failure, and others do not check input argument(s) at all.

Here is list of my findings:

GDALDestroyApproxTransformer: VALIDATE_POINTER0
GDALDestroyDriver: VALIDATE_POINTER0
GDALDestroyGCPTransformer: VALIDATE_POINTER0
GDALDestroyGenImgProjTransformer: VALIDATE_POINTER0
GDALDestroyGeoLocTransformer: no check, potential access violation
GDALDestroyRPCTransformer: no check, potential access violation
GDALDestroyReprojectionTransformer: VALIDATE_POINTER0
GDALDestroyTPSTransformer: VALIDATE_POINTER0
GDALDestroyTransformer: CE_Failure
GDALDestroyWarpOptions: VALIDATE_POINTER0

CPLFree: already no-op
GDALClose: already no-op
GDALDestroyColorTable: already no-op
GDALDestroyRasterAttributeTable: already no-op
GDALDestroyScaledProgress: already no-op
GDALDestroyWarpOperation: already no-op
OGRFree: already no-op
OGRGeocodeFreeResult: already no-op
VSIFree: already no-op
  1. Do we want to replace VALIDATE_POINTER0 with if nullptr do no-op strategy? I think it would be good to unify all GDALDestroy*, as are those at the bottom of the list above.
  1. The two with no-check, GDALDestroyGeoLocTransformer and GDALDestroyRPCTransformer, should be updated to either do VALIDATE_POINTER0 check or do no-op for nullptr input. Which one to go for?
  1. What to do with GDALDestroyTransformer?

comment:6 by Even Rouault, 8 years ago

I'd say to change all them to apply no-op strategy. Regarding GDALDestroyTransformer, it should be a no-op if receiving NULL, but still do the check for the GTI2 signature in the non null case.

comment:7 by Mateusz Łoskot, 8 years ago

Resolution: fixed
Status: newclosed

Finally, discussed changes applied in trunk @ r29207

comment:8 by Even Rouault, 8 years ago

Milestone: 2.0

comment:9 by Even Rouault, 7 years ago

Milestone: 2.02.0.0

Milestone renamed

Note: See TracTickets for help on using tickets.