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

Update Clang.jl generator scripts #120

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Update Clang.jl generator scripts #120

merged 2 commits into from
Aug 10, 2021

Conversation

melonedo
Copy link
Contributor

@melonedo melonedo commented Aug 9, 2021

I have updated the generator scripts for GDAL.jl, but I ended up finding most of the documentation are in cpp instead of header files, so I post-processed the code to recover doxygen docs in cpp.

Note that Clang.jl is patched (see JuliaInterop/Clang.jl#320)
https://github.com/melonedo/Clang.jl/tree/dump-doc-patch

cc @Gnimuc

@visr
Copy link
Member

visr commented Aug 9, 2021

Thanks a lot, I really appreciate it! Is there any chance that the separate header files still get written to their own julia files, like before? It would be nice to have the newly generated files in place of the old ones in this PR, such that we can see their diffs.

@Gnimuc
Copy link

Gnimuc commented Aug 10, 2021

It's hard to separate header files correctly because Clang.jl now supports forward decl, and a toposort needs to be applied to the DAG. As a result, the order of those identifiers in the C header files is no longer preserved. Although it's easy to separate functions and common definitions, I don't see any advantage of doing that.

@Gnimuc
Copy link

Gnimuc commented Aug 10, 2021

The new generator has been battle-tested on many projects: https://github.com/Gnimuc/GeneratorScripts. The quality of the generated code should be fine.

@visr
Copy link
Member

visr commented Aug 10, 2021

Ok thanks for explaining. We'll go with the single file wrapper. If I cut out the gdal_h.jl part and do a diff, it looks good, only small changes, like

function gdalcreate(hDriver, arg1, arg2, arg3, arg4, arg5, arg6)
becomes
function gdalcreate(hDriver, arg2, arg3, arg4, arg5, arg6, arg7)

and such. So in principle this should not be breaking right? I'll merge this into the clangjl branch and prepare a PR for master from there, where I'll also run ArchGDAL.jl tests, which have much better coverage.

@Gnimuc
Copy link

Gnimuc commented Aug 10, 2021

function gdalcreate(hDriver, arg1, arg2, arg3, arg4, arg5, arg6)
becomes
function gdalcreate(hDriver, arg2, arg3, arg4, arg5, arg6, arg7)

This is an intended improvement. The number n implies the argument is the n-th argument of the function, whereas the old behavior(n means the n-th un-named argument) looks very confusing in cases like:

function foo(arg1, x, arg2, y, z, arg3, arg4, arg5)

So in principle this should not be breaking right?

Yes. The default behavior of the new generator only contains improvements and bugfixes.

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