Relax null checking for certain methods

[yeesian]

I'm not sure if null checking should be performed (forcing downstream packages/users to wrap everything in a try-catch block) for cases in which you are expecting a null pointer to be returned. Case in point: https://github.com/visr/GDAL.jl/blob/master/src/ogr_api.jl#L3997-L4010

This came up when I was writing iterators for OGR feature layers, e.g. https://github.com/yeesian/GDALUtils.jl/blob/master/test/data1a.jl#L7-L13. I ended up using the C API instead: https://github.com/yeesian/GDALUtils.jl/blob/master/src/iteration.jl#L7-L8.

cc @visr @meggart

[visr]

Perhaps the "certain methods" is best decided by GDAL itself? If we remove checknull, we will still get a GDALError, but only if GDAL calls CPLError (right now even if it's only to report a CE_Warning).

I checked a few examples after setting checknull(x) = x.
Calling GDALOpen on a non existent file calls CPLError and thus throws. However GDAL.getdriverbyname for an unknown name just returns a null pointer, and so does your example, GDAL.getnextfeature if there is no next feature.

These examples seem reasonable, do you think we should just remove checknull?

[yeesian]

I think

• not checking for null pointers seems more like the exception rather than the rule, and
• making it the responsibility of users of (this package) to experience (and report) them seems like a good way of eliciting the range of edgecases, since
• they can always have the underlying C API functions as a fallback option

The alternative is to let users handle it for themselves, which is equivalent to not addressing the problem at a library-level. For now, I don't have a good understanding of how everything works with CPLErrors yet, and propose we come up with a list of methods for which

1. [Omission List] we will not perform null checking for now.
2. [OGRError List] we will perform OGR error checks (e.g. https://github.com/yeesian/GDALUtils.jl/search?utf8=%E2%9C%93&q=result+%21%3D+GDAL.OGRERR_NONE)

And update them as we go? I'll go ahead and prepare the lists based on what I've seen in GDALUtils if we have a consensus here. I'm afraid I've reached the end of my spring break and won't have time to do a more systematic/comprehensive sweep of all the methods for now.

Waiting on responses from @visr @meggart, and whoever wishes to chime in!

[meggart]

I don't have a very strong opinion on this one, but I tend to think we should rather remove checknull and leave the null checking to the user to avoid unnecessary try-catch blocks, which might have negative performance effect. As @visr said, if GDAL decides not to throw an error, we also shouldn't do this.

However, if we can come up with a good list of where nullchecks make sense and where they don't and adapt the functions accordingly, that's ok, too but probably more work.

[yeesian]

However, if we can come up with a good list of where nullchecks make sense and where they don't and adapt the functions accordingly, that's ok, too but probably more work.

I was originally reluctant about nullchecks in #3 (comment), but have since changed my mind after reading a lot of GDAL usage code out in the wild (they tend to result in a ton of boilerplate code). Coming up with a good list where nullchecks make sense etc is more work; but it is worthwhile work, and has the most benefit when it's close to the rewriting process after code-generation from Clang.

This package currently provides both options:

1. methods that perform null checks (amongst other things, e.g. loading pointers into bytestrings where appropriate)
2. methods that are the raw C API of GDAL

Some remarks:

• The user can avoid unnecessary try-catch blocks by switching to the alternatives in (2). The APIs for both (1) and (2) in this package are supposed to be inter-operable. File an issue otherwise.
• If we do not perform the nullchecks in (1), perhaps we should not load pointers into bytestrings/etc, and let the rewriting be a syntactic exercise in renaming the original GDAL method names?
• The effort of generating the list of methods with various expectations (when to expect a null check, etc) through issues/PRs will be offset by it being (i) a one-off effort for each method, and (ii) potentially useful knowledge for upstream GDAL developers when writing documentation and SWIG-ing code

If we let this package check null pointers by default, then it becomes a lot easier to surface 10% of the methods for which "C_NULL != error" (through usage and experience). If we switch off nullchecks, then we are stuck with the status quo of letting the users perform it for 90% of the methods (and downstream packages will individually still have to perform the task of identifying which methods to check null).

I have been working on GDALUtils to fuzz out most patterns of usage, so the remaining surface area should be diminishingly small, and makes it quite compelling for this package to check null pointers by default.

[yeesian]

Pinging @rouault if he has feedback.

For context: this package is a volunteer-based effort to provide a thin wrapper for GDAL usage in the Julia programming language, where we

1. use Clang.jl to generate a "low-level C API"
2. pull in the GDAL documentation to generate docstrings, and rewrite some of the methods for an intermediate level API. Example usage here: https://github.com/visr/GDAL.jl/tree/master/test (any of the files ending with .jl)
3. leave it to downstream packages to provide a higher level (fiona/rasterio-like) API. Examples here: https://github.com/yeesian/GDALUtils.jl/tree/master/test (any of the files ending with .jl)

We are wondering if there might be list(s) of methods for which

1. OGRErr will be emitted (and hence should be checked)
2. CPLError will be emitted (and should be checked; or whether GDAL will throw the error instead, and hence we should be wrapping it in a try-catch block)
3. pointers will be emitted where C_NULL should/should-not be interpreted as an error

and (if not) whether it might be possible to generate such list(s) from the gdal trunk.

[yeesian]

Relevant reading for ongoing art:

1. Improving GDAL error handling rasterio/rasterio#600
2. New pattern for handling GDAL errors in Cython code rasterio/rasterio#602

pinging @sgillies @perrygeo @brendan-ward for comments/feedback

[yeesian]

I'll maintain an ongoing list of references here for now:

• https://trac.osgeo.org/gdal/ticket/1775

[rouault]

OGRErr will be emitted (and hence should be checked)

Not sure to understand the question. OGRErr is a type, used as the returned value of functions, which you can check in the signatures, so not something "emitted".

CPLError will be emitted (and should be checked; or whether GDAL will throw the error instead, and hence we should be wrapping it in a try-catch block)

CPLError() can be called whether the return code of the function is successful or not, but I'd suggest that you can check with CPLGetLastErrorXXXX() when the return code is an error (or if you check always, do not error out if the error type was just a warning for example)

pointers will be emitted where C_NULL should/should-not be interpreted as an error

Do you mean if a NULL pointer as the return of a function should be interpreted as an error ? That depends on the context. If a pointer is returned, and it is NULL, I would check CPLGetLastErrorType() == CE_Failure to determine if an exception (or whatever the mechanism is in Julia) must be thrown or not

[yeesian]

I should have reworded "emitted" to "returned" for clarity.

Do you mean if a NULL pointer as the return of a function should be interpreted as an error ? That depends on the context. If a pointer is returned, and it is NULL, I would check CPLGetLastErrorType() == CE_Failure to determine if an exception (or whatever the mechanism is in Julia) must be thrown or not

I think that cleared up my confusion, thanks!