[fdo-dev] Checking new'ed pointers for NULL

[Pierre Dalcourt]

Hi Mateusz,

I think most of us agree with you.  The code that checks for a new
operator returning NULL is either old code or code that was written by
developers (including myself) before they realized that the new operator
on modern compilers would never actually return NULL.  :-)  However,
code that does the extra check is usually not harmful in itself so it
may not be worth spending the extra effort to remove them unless you
happen to be working on those files... 

Cheers,
Pierre

-----Original Message-----
From: Mateusz Loskot [mailto:mateusz at loskot.net] 
Sent: August 10, 2006 9:37 PM
To: dev at fdo.osgeo.org
Subject: [fdo-dev] Checking new'ed pointers for NULL

Hi,

I'd like to ask about the rationale of some testing pointers for NULL
I've noticed in FDO source code.

There are a few files, especially in fdordbms project
where pointers returned by 'operator new' are tested for NULL.
Here is a sample from FDO sources:

FdoPtr<FdoEnvelopeImpl> envelope = new FdoEnvelopeImpl();
if (envelope == NULL)
   throw FdoException::Create(
            FdoException::NLSGetMessage(FDO_NLSID(FDO_1_BADALLOC)));



My question is what's the reason of these tests?
The only reason I can see is requirement to support old
C++ compilers that does not do automatic nullptr tests
in allocation functions.
(or compilers without exceptions support, e.g. on embedded systems,
but that's not the case here, because FDO uses exceptions extensively)

According to C++ Standard (3.7.3.1 Allocation functions,
3.7.3.1 Allocation functions) 'operator new' is binding to following
requirement:

"Required behavior: Return a non-null pointer to suitably aligned
storage (3.7.3), or else throw a bad_alloc exception. This requirement
is binding on a replacement version of this function."

It means 'operator new' never returns null pointer if fails to allocate
memory (unlike malloc in C), but throw std::bad_alloc exception.

The only version that returns null pointer on allocation failure is
nothrow version of 'operator new', but it should be used explicitly.



In other words, checking new'ed poniters for null instead of catching
std::bad_alloc exception should be considered as a BUG.
The 'if' clause is never executed, because on allocation failure the
std::bad_alloc exception thrown by new will skip the control to matching
catch() clause (if is provided) or will call terminate function and
program will exit with error.

So, all those null pointer tests are not needed but IMHO tedious because
they make code less readable.



The standard compliant and correct verion of snippet presented above is:

try
{
   FdoPtr<FdoEnvelopeImpl> envelope = new FdoEnvelopeImpl;
}
catch (std::bad_alloc &b)
{
   // If FdoException is prefered, re-throw it here
   throw FdoException::Create(
         FdoException::NLSGetMessage(FDO_NLSID(FDO_1_BADALLOC)));
}

Another option, is to use nothrow version of new, keep test for null
and throw FdoException:

FdoPtr<FdoEnvelopeImpl> envelope = new(std:::nothrow) FdoEnvelopeImpl;
if (envelope == NULL)
{
   throw FdoException::Create(
            FdoException::NLSGetMessage(FDO_NLSID(FDO_1_BADALLOC)));
}



Here I grepped out a list of files which include the null pointer test
discussed above:

Fdo/Unmanaged/Src/Geometry/EnvelopeImpl.cpp
Fdo/Unmanaged/Src/Spatial/MathUtility.cpp
Providers/GenericRdbms/Src/Fdo/Filter/FdoRdbmsFilterProcessor.cpp
Providers/GenericRdbms/Src/Fdo/Pvc/FdoRdbmsPvcInsertHandler.cpp
Providers/GenericRdbms/Src/Fdo/Connection/FdoRdbmsPropertyDictionary.cpp
Providers/GenericRdbms/Src/Fdo/Schema/FdoRdbmsSchemaUtil.cpp


p.s. I decided to not to report a defect but push it for
discussion first.

Best regards
-- 
Mateusz Loskot
http://mateusz.loskot.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe at fdo.osgeo.org
For additional commands, e-mail: dev-help at fdo.osgeo.org