Catch NULL strings (and others?) that need to be non-NULL with a special typemap

Howard,

Do you have any ideas how to implement this? Ari?

Here are some thoughts from the #gdal:

[23:46]  <hobu> re #1775 we can make a swig macro much like mloskot_dogwalk did for the c api 
[23:46]  <gdaltrac> Ticket #1775: Catch NULL strings (and others?) that need to be non-NULL with a special typemap, http://trac.osgeo.org/gdal/ticket/1775
[00:00]  * You are now known as mloskot
[00:02]  <mloskot> re 1775, in my opinion if there are requirements defined about a function arguments and these reqs have to be always fulfilled then these cases are candidates for assertions
[00:02]  <mloskot> ie. 
[00:02]  <mloskot> foo(int* p);
[00:03]  <mloskot> if p is *always* expected to be non-NULL, then 
[00:03]  <mloskot> assert(0 != p);
[00:03]  <mloskot> is strongly recommended as the first instruction of foo() function
[00:04]  <mloskot> Certainly, we can replace assert() with our own macro being active in non-DEBUG builds
[00:05]  <mloskot> but we have to be aware of potential performance penalties
[00:05]  <Nowak> mloskot: you should add "as long as it doesnt negatively affect performance"
[00:05]  <Nowak> heh, you did :D

IOW, I think input parameters should be categorized and prioritized in some way.

I suggest a new typemap:

%typemap(in) (char *non_null_input)

This typemap should catch fatal nulls, e.g. in the case of Perl bindings:

%typemap(in) (char *non_null_input)
{
 /* %typemap(in) (char *non_null_input) */
 if (!$1) {
   croak("argument must not be null");
   SWIG_fail;  }
} 

This typemap should be used in all those places where nulls are fatal if given to the C API, for example:

 %apply (char *non_null_input) {const char *pszNewDesc};
 void SetDescription( const char *pszNewDesc ) {
   GDALSetDescription( self, pszNewDesc );
 }
 %clear const char *pszNewDesc; 

Would it be less invasive to change Perl's char* typemap to not pass NULLs into GDAL functions rather than doing the %apply/%clear dance around every function in the interface?

Isn't this issue a problem in other languages? If it is not then of course apply/clear creates too much noise.

In some cases NULL pointers are a valid choice to GDAL but probably most cases can be cought with language-specific typemaps.

For example, here's some interactive output with SetDescription?.

>>> ds = gdal.Open('junk.wcs')
>>> ds.SetDescription()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/Users/hobu/svn/gdal/swig/python/gdal.py", line 154, in SetDescription
    return _gdal.MajorObject_SetDescription(*args)
TypeError: MajorObject_SetDescription() takes exactly 2 arguments (1 given)
>>> ds.SetDescription('')
>>> ds.GetDescription()
''
>>> ds.SetDescription('some crap')
>>> ds.GetDescription()
'some crap'
>>> ds.SetDescription(None)
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct NULL not valid
Abort trap

So, it is a problem here too.

I can fix all these problems with 10 specific typemaps in typemaps_perl.i, these are of type:

%typemap(check) (const char *pszNewDesc)
{
  /* %typemap(check) (const char *pszNewDesc) */
  if (!$1) {
    croak("Description must not be undefined");
    SWIG_fail;  
  }
}

Note "check" instead of "in". These do not break the usual tests I have, so perhaps they do not break other things.

Frank,

Ari's solution is fairly elegant, and I would like to apply this to the Python bindings as well. The catch is that we would always throw an exception. In the past these were always crashes, so I don't think it is too bad of an approach to take. Do you agree?

The solution is quite sensitive to argument names, which has bitten me already a couple of times. The argument name policy should be enforced somehow.

Ari

I don't really understand why this is necessary when we have null checking in the C API. Note that the C API returns an error which is turned into an exception by most swig languages *except if GDAL is built with -DDEBUG* in which case it is a fatal error (to ensure fast bug finding).

However, if you guys want extra "stuff" at the SWIG level I'm not going to interfere.

I wish it had been decided before I tasked Mateusz to put in all the C API argument checking though.

In fact I agree. I'll probably remove it more or less completely _once_ we have null checking in the C API.

I think we made great progress for 1.5 between the typemaps and C API checks. This effort isn't totally done, but we should push off these kinds of changes until 1.6 now.