Upgrade the generator script to test Clang.jl's new generator

[Gnimuc]

No description provided.

[codecov]

Codecov Report

Merging #39 (808ec72) into master (3134e6b) will decrease coverage by 39.38%.
The diff coverage is 26.41%.

❗ Current head 808ec72 differs from pull request most recent head 8dcb646. Consider uploading reports for the commit 8dcb646 to get more accurate results

@@             Coverage Diff             @@
##           master      #39       +/-   ##
===========================================
- Coverage   73.80%   34.42%   -39.39%     
===========================================
  Files           3        3               
  Lines          84       61       -23     
===========================================
- Hits           62       21       -41     
- Misses         22       40       +18     

Impacted Files	Coverage Δ
src/operators.jl	25.00% <25.00%> (ø)
src/CEnum.jl	25.64% <25.64%> (-44.36%)	⬇️
src/LibVulkan.jl	80.00% <50.00%> (-12.31%)	⬇️
src/VulkanCore.jl

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3134e6b...8dcb646. Read the comment docs.

[serenity4]

Looks clearer! While quickly browsing through the changes, a few things got my attention.

I saw that all handle types (VkFence_T for example) become mutable structs instead of Cvoid, is there a benefit to that? It would be heavier for compilation, better for dispatch I guess, and why make them mutable?

Also, what happened with some structs such as VkImportMemoryWin32HandleInfoKHR? On master it reads

struct VkImportMemoryWin32HandleInfoKHR
    sType::VkStructureType
    pNext::Ptr{Cvoid}
    handleType::VkExternalMemoryHandleTypeFlagBits
    handle::Cint
    name::Cint
end

against struct VkImportMemoryWin32HandleInfoKHR end from the new generator, as if it just skipped the fields.

[Gnimuc]

I saw that all handle types (VkFence_T for example) become mutable structs instead of Cvoid, is there a benefit to that? It would be heavier for compilation, better for dispatch I guess, and why make them mutable?

This behavior is now configurable with the new generator. The full configuration list can be found here. As of the mutable part, it prevents the compiler from doing some aggressive optimizations. In practice, there is no much difference since opaques should only be used in the form of an opaque pointer.

Also, what happened with some structs such as VkImportMemoryWin32HandleInfoKHR? On master it reads

Nice catch! That's definitely a bug of the new generator. UPDATE: this is addressed on the Clang.jl side.

[Gnimuc]

hi @serenity4, I just added a new feature that all "top-level structs"(structs that are not served as a field type of any other structs, so it's safe to mark them mutable) can be automatically generated as mutable struct. Could you help to check whether this change will affect Vulkan.jl or not? Any feedback would be great!

[Gnimuc]

Also, any thoughts on the CI failure on Linux and Windows?

OK, just found the reason. It turns out that if a function accepts a Ptr{top-level-struct}, we can no longer pass Vector{top-level-struct} because the top-level-struct is not an isbits type and we should use Ref{NTuple{n,top-level-struct}} instead.

[serenity4]

Ah, indeed in that case it's better to have them non-mutable. What was the reason for introducing this feature?

[Gnimuc]

I find in some cases(e.g. init and fill VkInstanceCreateInfo in a C style), it's convenient to have a mutable struct with auto-generated init constructor.

[serenity4]

Maybe we can do this for the *[Create/Allocate]Info structs that are never passed/returned as a vector (including extensions). To be more future proof, we could include this as a package option with the default being immutable structs. But I'm afraid this would cause more trouble than it's worth. Personally I don't have any use cases for this (since Vulkan.jl provides defaults already), but if you do feel free to come up with a plan.

[Gnimuc]

Now I implemented a more strict rule for this feature and exposed whitelist&blacklist for manually tweaking. Basically, the necessary condition is

this type is not used as a field type in any other types

and I added a more strict rule to exclude the situation where the type needs to be passed a la a Vector:

if this type is used as an argument type(in form of a Ptr{}) but none of the other argument types is an integer type.

here I'm assuming if a C function expects a vector input argument, it needs a pointer + a size integer.

For the sake of safety, I agree we should use immutable struct as default, but I'd like to explore how far we can go with mutable structs as many as possible.

[serenity4]

if this type is used as an argument type(in form of a Ptr{}) but none of the other argument types is an integer type.

This kind of logic reminds me of Vulkan.jl, where there is basically some kind of IR (generated from the spec) that is used to generate the wrapper. There is a function that tells you whether a parameter/field is used as a vector, by checking whether its spec type is a pointer and if there is an integer (except size_t, which is for data) len parameter attached.
Vulkan.jl was designed to work with immutable types, so this C-like way of setting fields would require some work; however I feel like this is a wrapper feature which has nothing to do with the logic that VulkanCore.jl should just minimally interface with the C library.

Thinking about it, if some structs are mutable and others not, depending on the proportion of immutable structs the user may not want to bother checking whether the type is mutable, and just not use the feature. As in, there would be 1 chance out of 10 that the pattern may not work (due to a particular struct being immutable), and when using VulkanCore.jl directly any uncaught error is very likely to cause a segfault (due to finalizers being run in an unordered fashion at an unexpected point, or resources that were not cleaned up before GC if finalizers aren't used for that).

So I am a bit skeptical about having this feature at the core. I'm still not against having it as a package option, but only if there is a concrete use in line with the goal of the library and if the limitations are made known to the user.

[Gnimuc]

Thanks for the feedback. That makes sense to me but I don't have time to make it a package option at the moment, so just disable this behavior for VulkanCore.jl.

[Gnimuc]

@serenity4 may I ask why you bundled the old version of CEnum.jl in #42 ?

[Gnimuc]

This needs to be bumped.

[serenity4]

Hmm it wasn't done on purpose. I just ran the generator script, which produced the CEnum.jl file, and assumed it was good. If that was an old version then we need to update it.

[Gnimuc]

OK, this is ready for a review. Feel free to merge this if there are no other problems. It's too late at my local time, I'm going to sleep. :)

[serenity4]

Looks good to me. I can't test it yet with Vulkan.jl (will need some changes because I just expected everything to be included until now) and I trust you with the setup of the generator, otherwise I didn't see anything to change. That's really awesome, thanks for the work! 💯

I'll let you merge it, I don't know if you want to squash this or keep the full commit history.