JuliaInterop / Clang.jl

C binding generator and Julia interface to libclang
https://juliainterop.github.io/Clang.jl/
MIT License
217 stars 68 forks source link

Clang 16 fixes #472

Closed JamesWrigley closed 4 months ago

JamesWrigley commented 4 months ago

This is a speculative attempt to fix Clang 16 compatibility. My understanding of the problem (helped by the folks on the LLVM Discourse) is that Clang 16 represents way more types as ElaboratedType's than before: https://github.com/llvm/llvm-project/commit/15f3cd6bfc670ba6106184a903eb04be059e5977

This PR fixes that by looking at the named type instead in most places. It's probably worth testing this on some bigger projects because I'm not fully confident that it's doing the right thing (it's also a little hideous to normalize the types everywhere :frowning_face: ).

I haven't figured out how to do it yet, but I think what we really want to do is determine whether an ElaboratedType has any elaboration and then desugar it if so. That seems like the 'right' thing to do compared to blindly desugaring for everything but enums.

EDIT: two odd things:

Gnimuc commented 4 months ago

I pushed a similar fix in https://github.com/JuliaInterop/Clang.jl/pull/465/commits/bcab1c79d6fe23bd0ba8520da1eecf47f406d0ff.

The following chunk of code basically reads:

https://github.com/JuliaInterop/Clang.jl/blob/e1a96aeeb20ee7e89a14c4376ab2311e7b9b1865/src/generator/resolve_deps.jl#L24-L28

https://github.com/JuliaInterop/Clang.jl/blob/e1a96aeeb20ee7e89a14c4376ab2311e7b9b1865/src/type.jl#L270-L287

so, the old has_elaborated_reference should be has_elaborated_tag_reference. Note we need to recursively check the leaf referenced type is a tag type.

Gnimuc commented 4 months ago

I suspect the documentation failure is related to the following contents. The range of the source code and comments are somehow miss-calculated for "elaborated" cursors.

3) This patch could expose a bug in how you get the source range of some TypeLoc. For some reason, a lot of code is using getLocalSourceRange(), which only looks at the given TypeLoc node. This patch introduces a new, and more common TypeLoc node which contains no source locations on itself. This is not an inovation here, and some other, more rare TypeLoc nodes could also have this property, but if you use getLocalSourceRange on them, it's not going to return any valid locations, because it doesn't have any. The right fix here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive into the inner TypeLoc to get the source range if it doesn't find it on the top level one. You can use getLocalSourceRange if you are really into micro-optimizations and you have some outside knowledge that the TypeLocs you are dealing with will always include some source location.

JamesWrigley commented 4 months ago

Neat, thanks :heart: I think I figured out the documentation issue too, looks like we're hitting this bug: https://github.com/JuliaLang/julia/issues/52986 Fixed in 08b1ad2. I think both this and #465 can be merged now :crossed_fingers: (I removed my hacky fix)

Gnimuc commented 4 months ago

Sorry. 😓 I did a rebase merge. Could you rebase this onto the master branch?

JamesWrigley commented 4 months ago

No worries, will do :)