return nothing for NULL strings

[visr]

Fixes #67.

Currently this errors with ArgumentError: cannot convert NULL to string in unsafe_string, even though many functions are documented to return NULL in certain cases, that are not errors. This now works similarly to many Base functions like findfirst, that return nothing in case nothing is found.

[evetion]

A few comments:

• Is the name unsafe_string_or_nothing better?
• Maybe we could document this change somewhere?
• You could also return an empty string for example, as that is at least the same type.
• Does the C_NULL indicate different things for different function (i.e. an error or nothing)?

[visr]

Thanks, your comments made me think about this again, and try something different. First my replies to your points:

Is the name unsafe_string_or_nothing better?

From the unsafe_string docs: "This function is labeled "unsafe" because it will crash if p is not a valid memory address to data of the requested length." In this case I wouldn't call it unsafe since we know that GDAL allocated this memory.

Maybe we could document this change somewhere?

Yeah, I think we just need to make a changelog for going from v0.2 to v1.0. I'd prefer at this point to do that all at once I think. (Going forward we should keep it up to date)

You could also return an empty string for example, as that is at least the same type.

Yeah but then you cannot distinguish it from an actual empty string result. And since this method is used by Base as well it seems like a better choice.

Does the C_NULL indicate different things for different function (i.e. an error or nothing)?

It seems like it, from the docs:

• CPLSerializeXMLTree: the document on success or NULL on failure.
• VSIGetFileSystemOptions: a string, which must not be freed, or NULL if no options is declared.

Especially the last point made me think, currently we only check for errors when we get a Ptr{Cvoid} back from GDAL that is a NULL pointer. Digging back a little, to Even Rouault's comment in #19 (comment):

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)

So the new commit here effectively switches to always checking for errors, but only throwing on CE_Failure. I took out CE_Fatal from the error checking codes since this is not in need of checking anyway, if this happens, it also crashes julia, just try running GDAL.cplemergencyerror("things are bad") if you can lose your julia session.

This also introduces a list of functions that return Cstring and are known to need their memory freed. For now I only put in gdalinfo. But looking around at some of the docs, see below, I noticed how it's really a mix of yes/no on freeing. So I believe the (current) default of not freeing is fine, will leak some memory. As we populate this list (string_free_me in wrap.jl), we can make it a little less leaky along the way.

• CPLStrdup: pointer to a newly allocated copy of the string. Free with CPLFree() or VSIFree().
• CPLGetPath: Path in an internal string which must not be freed. The string may be destroyed by the next CPL filename handling call. The returned will generally not contain a trailing path separator.
• OGR_L_GetName: the layer name (must not been freed)

[yeesian]

I ran into something similar with https://github.com/JuliaGeo/Proj4.jl/blob/882b48a9ea75198bc3325a35a93cac36e3f1ceec/test/proj6api.jl#L11-L13 and agree with the motive of this PR, but might need a few days to properly review it (if my input is needed).

[visr]

Yeah the main reason I requested your review was not to check if I implemented my ideas correctly, Maarten could check that as well, but to make sure you are on board with this direction. If you'd like to go over it properly, that's also welcome of course, no need to rush this in.

Even if this doesn't nail it 100%, we can easily opt out of checking for errors or free more strings going forward without breaking code. To me it was a good sign that I didn't have to change any tests, just add one for the returning nothing for NULL strings part.

[evetion]

I very much like the approach we're taking here. Just a few naming issues left.

[evetion]

So the new commit here effectively switches to always checking for errors, but only throwing on CE_Failure. I took out CE_Fatal from the error checking codes since this is not in need of checking anyway, if this happens, it also crashes julia, just try running GDAL.cplemergencyerror("things are bad") if you can lose your julia session.

This seems it's worth its own issue? From testing (on stable GDAL), it can handle one emergencyerror, but not two.

julia> using GDAL

julia> GDAL.cplemergencyerror("things are bad")
ERROR: GDALError (CE_Fatal, code 1):
	things are bad

Stacktrace:
 [1] gdaljl_errorhandler(::GDAL.CPLErr, ::Int32, ::Cstring) at /Users/me/.julia/packages/GDAL/VqKWA/src/error.jl:26
 [2] cplemergencyerror(::String) at /Users/me/.julia/packages/GDAL/VqKWA/src/cpl_error.jl:14
 [3] top-level scope at none:0

julia> GDAL.cplemergencyerror("things are bad")
FATAL: things are bad

signal (6): Abort trap: 6
in expression starting at no file:0
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 1954108 (Pool: 1953578; Big: 530); GC: 3
[1]    81882 abort      /Applications/Julia-1.1.app/Contents/Resources/julia/bin/julia

[visr]

This seems it's worth its own issue? From testing (on stable GDAL), it can handle one emergencyerror, but not two.

Yeah I noticed the same thing. Though I believe it's not even supposed to be able to handle it once? See also the comment here:

GDAL.jl/test/error.jl

Lines 42 to 54 in 147f624

	# Quoting cpl_error.cpp regarding CE_Fatal:
	# > The default behaviour of CPLError() is to report errors to stderr,
	# > and to abort() after reporting a CE_Fatal error. It is expected that
	# > some applications will want to suppress error reporting, and will want to
	# > install a C++ exception, or longjmp() approach to no local fatal error
	# > recovery.
	# The abort means we cannot catch CE_Fatal GDALErrors.
	# Interestingly, the following works once:
	# @test_throws GDAL.GDALError GDAL.cplemergencyerror("things are bad")
	# But if you run that line a second time, it quits julia with:
	# FATAL: things are bad
	# signal (22): SIGABRT
	# So let's not even test it once to be safe

Could be it's own issue, but I've never encountered a CE_Fatal, so it's mostly hypothetical at this point.