Alter the generator to free strings returned by GEOS.

[nraynaud]

I have a memory leak in my use of GEOS, and I studied freeing strings as an hypothesis, so here is the code.

Sadly, I couldn't find my leak.

[visr]

Too bad this didn't solve your issue. What are your thoughts about merging this? Do you know that this is always safe?

Right now an exception is made for GEOSVersion since it is the only function from http://libgeos.org/doxygen/geos__c_8h.html that returns const char *. Do you know if it is easy to detect that return type instead of using the name, in case more such functions get added in the future?

Here are some related links for GDAL.jl: https://github.com/JuliaGeo/GDAL.jl/issues/77 https://github.com/JuliaGeo/GDAL.jl/blob/8ffa73af0da91e3aa54163052ddb1369ebaf3682/gen/generator.jl#L61-L65 https://github.com/JuliaGeo/GDAL.jl/blob/d44dc737ec16a60d1edb81bb71e0cb5f0666f165/src/error.jl#L65-L75

[nraynaud]

Hi Martijn, I am as circumspect as you are about this PR TBH. I think it's a step in the right direction, but I don't know if it's enough. For the const char*, I searched the GEOS.h documentation. I couldn't find how to interrogate the return types in the clang library. The part I neglected is the strings returned in a pointer parameter (GEOSisValidDetail() Is the only instance I think) I asked some friends about injecting custom free/malloc in GEOS for testing, nobody came up with a way that wouldn't also inject it in the julia environment.

I you are interested in this PR, I can try to improve it until it's good enough to be merged.

Maybe you or some onlookers have suggestions to improve it.

Nico.

[nraynaud]

Hi @visr I think I'm ready for a round of review.

[visr]

This looks quite good, thanks for going through the extra effort to automatically detect when to free strings. That might be useful in other wrappers as well.

I rebased the branch, and if you don't mind renamed to string_copy_free to make it more explicit what it does. I also added the optional argument for the context handle. Could you perhaps, for the _r functions that get the handle as an argument, forward that handle to the string_copy_free argument? Otherwise you could have say a user creating their own context handle and calling GEOSRelate_r(handle, g1, g2), only for it to still use the global context when freeing the resulting string, which (I assume) would not be what a user would excpect. Besides that, I think this is good to go!

[nraynaud]

yes, I spent an inordinate amount of time looking into how to use clang, but I think it gives a good hint for other people who want to do things with types, I couldn't find any example anywhere.

I'll takle the context pointer soon.

[visr]

@Gnimuc do you think https://github.com/JuliaGeo/LibGEOS.jl/blob/6fd06dbc7994bb4a221c4bac53f2c5a9a42e31b7/gen/generator.jl is worth linking to in https://github.com/JuliaInterop/Clang.jl#examples? The rewrite function in which we use MacroTools to match and decompose functions for rewriting is quite a convenient pattern, and the newly added return_type_is_const_char could perhaps be useful for others. https://github.com/JuliaGeo/GDAL.jl and https://github.com/JuliaGeo/Proj.jl are similar.

[nraynaud]

I am ready for a next round of review. I unwrapped the pointer for the context, because from the low level we only have access to the pointer, not the full object.

[visr]

Sorry for letting this slip @nraynaud. Thank you for your effort!