Conversation
|
Thanks! I'll review shortly. As you probably know, there is no "user base" for this package yet, so feel free to "break" anything you think should be improved. |
Also add a struct definition to be able to conveniently document the various bits of output data.
This should remove some gnarly-looking underscores.
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
LGTM. A few things to think about, and then merge when you feel ready.
| end | ||
|
|
||
| function build_type_graph(typedescs::OrderedDict{Int, TypeDesc}; | ||
| pointer_filter::Function) |
There was a problem hiding this comment.
Any reason not to make pointer_filter the first positional argument so that one could use a do block? https://docs.julialang.org/en/v1/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia-Base
There was a problem hiding this comment.
I find that a bit more opaque at the call-site TBH, if the function name does not clearly communicate what it's going to use the function for
| forward_declared::BitSet | ||
| # A vector of exposed functions from the imported ABI | ||
| entrypoints::Vector{MethodDesc} | ||
| end |
There was a problem hiding this comment.
Would this benefit from having a Base.show method? And should the comments be moved into a docstring?
There was a problem hiding this comment.
Nothing fancy, but this is something:
ABIInfo(...) object, with 12 types and 3 entrypoints.
Types:
∘ Float32
∘ Ptr{Float32}
∘ Int64
∘ Ptr{CTree{Float64}}
∘ Int32
∘ CVector{Float32}
∘ CVectorPair{Float32}
∘ MyTwoVec
∘ Ptr{MyTwoVec}
∘ CVector{CTree{Float64}}
∘ Float64
∘ CTree{Float64}
Entrypoints:
∘ tree_size(tree::CTree{Float64})
∘ copyto_and_sum(fromto::CVectorPair{Float32})
∘ countsame(list::Ptr{MyTwoVec}, n::Int32)
- Add `Base.show(::IO, ::ABIInfo) - Add doc-comments for `ABIInfo` - Add addl. fields to PrimitiveTypeDesc - Update test ABI JSON file to latest output
There was a problem hiding this comment.
Apologies for the significant delay - this should have been done months ago!
I've been assuming this PR needed significant re-work for some reason, but the updates from JuliaLang/julia#59108 ended up very easy. I think I've covered all existing feedback now, and this appears to be working with the ABI export landed upstream.
@timholy Do you remember if there were any other major TODO's from our prior conversation(s)?
| end | ||
|
|
||
| function build_type_graph(typedescs::OrderedDict{Int, TypeDesc}; | ||
| pointer_filter::Function) |
There was a problem hiding this comment.
I find that a bit more opaque at the call-site TBH, if the function name does not clearly communicate what it's going to use the function for
| forward_declared::BitSet | ||
| # A vector of exposed functions from the imported ABI | ||
| entrypoints::Vector{MethodDesc} | ||
| end |
There was a problem hiding this comment.
Nothing fancy, but this is something:
ABIInfo(...) object, with 12 types and 3 entrypoints.
Types:
∘ Float32
∘ Ptr{Float32}
∘ Int64
∘ Ptr{CTree{Float64}}
∘ Int32
∘ CVector{Float32}
∘ CVectorPair{Float32}
∘ MyTwoVec
∘ Ptr{MyTwoVec}
∘ CVector{CTree{Float64}}
∘ Float64
∘ CTree{Float64}
Entrypoints:
∘ tree_size(tree::CTree{Float64})
∘ copyto_and_sum(fromto::CVectorPair{Float32})
∘ countsame(list::Ptr{MyTwoVec}, n::Int32)
|
Ready to merge? |
|
Good to go from my POV |
3 participants
Expect some more touch-ups on Monday, but this should be the core pieces.
There are some significant changes to how mangling works in here, but hopefully this feels mostly in-spirit with the existing behavior. As a bonus, we now support recursive types! (which require forward declarations / pointer-indirection in C, of course)