-
Notifications
You must be signed in to change notification settings - Fork 0
Adjust to new JSON ABI format #2
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
base: main
Are you sure you want to change the base?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A few things to think about, and then merge when you feel ready.
@@ -5,10 +5,12 @@ authors = ["Tim Holy <[email protected]> and contributors"] | |||
|
|||
[deps] | |||
Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6" | |||
JSON = "682c06a0-de6a-54ab-a142-c8b1cf79cde6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON3/StructTypes might automate some of the json->julia parsing, but I am not sure there would be any real gain and this seems fine to me.
@@ -3,10 +3,10 @@ module JuliaLibWrapping | |||
using OrderedCollections | |||
using Graphs | |||
|
|||
export parselog, wrapper | |||
export CProject | |||
export import_abi_info, wrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an improvement in naming, thanks
|
||
struct PrimitiveTypeDesc | ||
# TODO: support custom primitive types? | ||
name::String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to include bits
?
end | ||
|
||
function build_type_graph(typedescs::OrderedDict{Int, TypeDesc}; | ||
pointer_filter::Function) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
# therefore have to use forward-declarations instead of just sorting declarations. | ||
recursive_types = BitSet() | ||
|
||
full_type_graph = build_type_graph(typedescs; pointer_filter = (id)->true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full_type_graph = build_type_graph(typedescs; pointer_filter = (id)->true) | |
full_type_graph = build_type_graph(typedescs; pointer_filter = Returns(true)) |
end | ||
|
||
""" | ||
entrypoints, typedescs, forward_declarations = parselog(filename::String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entrypoints, typedescs, forward_declarations = parselog(filename::String) | |
abi_info = import_abi_info(filename::String) |
forward_declared::BitSet | ||
# A vector of exposed functions from the imported ABI | ||
entrypoints::Vector{MethodDesc} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this benefit from having a Base.show
method? And should the comments be moved into a docstring?
@test collect(keys(sorted)) == Int[5,2,1,4,3] | ||
else | ||
@test false # unexpected forward declarations | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
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)