Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Clang to wrap PROJ 6.1.0 #27

Merged
merged 14 commits into from
Nov 19, 2019
Merged

use Clang to wrap PROJ 6.1.0 #27

merged 14 commits into from
Nov 19, 2019

Conversation

visr
Copy link
Member

@visr visr commented May 31, 2019

I plan to finish JuliaGeo/GDAL.jl#66 first, but since that is now stuck on PROJ_LIB related issues, it's convenient to have the new API at hand. Lessons learned there will probably have to be applied here too.

ref https://discourse.julialang.org/t/setting-env-for-a-shared-library-in-init/24724/

@c42f
Copy link
Member

c42f commented May 31, 2019

Very cool 🎉

@visr
Copy link
Member Author

visr commented Aug 13, 2019

Ha, with the new rewriter it becomes much more fun to rewrite wrapped functions. In d1d4bb5 I made all ctx arguments keywords, also updating the order of parameters in the docstrings (though that means they no longer match C).

Here is the doxygen.xml.gz for if anyone wants to run it.

We could take it further and do this for all essentially optional parameters. That would make some functions much easier to use, for instance proj_coordoperation_get_param:
https://github.com/JuliaGeo/Proj4.jl/blob/d1d4bb5bbc624442abf830362f4c02d22cee3081/src/proj_c.jl#L1345-L1382

Some things left to do:

  • add tests to see if this new API is actually working
  • __init__: create a context
  • __init__ point it to proj.db, similarly to GDAL but using PROJ's proj_context_set_search_paths.
  • use error reporting. I think we should always check automatically for all ccall that return Ptr{PJ}. If it is an error, we get the error string and throw a PROJError. (Check specifically proj_get_id_code, which returns a string, or NULL in case of error or missing name. Right now that leads to ArgumentError: cannot convert NULL to string.

visr and others added 6 commits November 15, 2019 21:56
This code is mostly copy pasted from JuliaGeo/GDAL.jl#73.

Three transformations are applied:

- docstrings are added, created from PROJ Doxygen XML output
- functions that return a Cstring are wrapped in unsafe_string to return a String
- functions that return a Ptr{Cstring} are wrapped in unsafe_loadstringlist to return a Vector{String}
C_NULL means default context
* add tests

* reorder the tests
adds space to `ctx=C_NULL`
This handler, `log_func`, is registered in `__init__`. It simply throws 
when the level is at error, and ignores otherwise. Based on 
pyproj4/pyproj#215.
Start using a generic `aftercare` function just like in GDAL.jl.
They always have defaults when C_NULL is passed, so we can just use 
keyword arguments here.
@visr visr marked this pull request as ready for review November 17, 2019 15:04
@visr visr requested a review from yeesian November 17, 2019 15:04
@visr
Copy link
Member Author

visr commented Nov 17, 2019

Ok I still added some error handling support. If PROJ throws an error that is turned into a Julia exception. Note that I am currently not checking each Ptr{PJ} that is returned from ccall, since I'm not sure if it is needed, and could not find a testcase that would not already have PROJ throw an error here.

Furthermor I still attempted to make using this interface a bit more pleasant, by turning some arguments into keyword arguments defaulting to C_NULL, when the docs stated this was accepted as well.

Test coverage is not yet great, but what we did test works well, and actually I think this is large enough a PR as is. @yeesian do you think this is ok to merge? No rush.

@yeesian
Copy link
Member

yeesian commented Nov 17, 2019

This is incredible work, thanks!

Yeah I think it is god enough to be merged; we can increase coverage after the fact (like with LibGEOS)

@visr
Copy link
Member Author

visr commented Nov 17, 2019

it is god enough

High praise indeed! Ok merging within a day or so, unless someone else wants to have a look.

Copy link
Member

@yeesian yeesian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Some minor comments.

test/proj6api.jl Outdated Show resolved Hide resolved
test/proj6api.jl Outdated Show resolved Hide resolved
src/error.jl Outdated Show resolved Hide resolved
src/Proj4.jl Outdated Show resolved Hide resolved
gen/wrap_proj.jl Outdated Show resolved Hide resolved
@visr visr mentioned this pull request Nov 18, 2019
@visr
Copy link
Member Author

visr commented Nov 18, 2019

Thanks for the comments Yeesian, much appreciated. Addressed them in the latest commit.

@visr visr merged commit b875551 into master Nov 19, 2019
@yeesian yeesian deleted the wrap6 branch November 20, 2019 04:07
@visr visr mentioned this pull request Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants