-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add support for automatically calling unsafe_load() in getproperty() #502
base: master
Are you sure you want to change the base?
Conversation
f9a668d
to
2e7e00d
Compare
The problem JET found was in how bitfields are handled, I believe that's fixed now. My approach was to:
One downside I noticed when playing around with the new wrappers in CImGui is that when we want to write to the struct we really do want to get a pointer instead of # io.ConfigFlags is now a Cint, but we really want a Ptr{Cint}
CImGui.CheckboxFlags("io.ConfigFlags: NavEnableKeyboard", io.ConfigFlags, CImGui.ImGuiConfigFlags_NavEnableKeyboard) The simple fix there would be to use On a side note, turns out that all of our wrappers are fantastically inferrable so JET is very good at finding problems with them :) |
What's the behavior of loading a chain of pointers? |
There's an example of that in the tests with |
I'm pretty sure this gonna confuse newbies, probably LLMs too... |
see https://giordano.github.io/blog/2019-05-03-julia-get-pointer-value/ for a hack of dereference operator in Julia. |
I actually think it'll make it easier for newbies since they can blindly access nested properties and always get the right result. A good example of this is something like an struct Foo
vec::ImVec2
end Currently to access vec = unsafe_load(obj.vec)
vec.x, vec.y Whereas with this PR they can simply do
That's interesting, but it still makes reading fields harder than writing to them. In my experience fields are read more often than written to, so I think it makes sense to sacrifice some write syntax for the sake of making reads simpler. |
hmm... I kinda agree, but if some packages adopt one rule but not the other, there could be a Vim-vs-Emacs war in the future... 😄 however, I don't think that's something we can control. and it's not easy to implement this feature out of the source tree, so it's ok to merge I guess. |
I would actually like to make this the default at some point in the future 👀 But that would be wildly breaking so we should probably wait to see if people actually like it first. But getting a pointer to a field is still a bit tricky :\ What do you think about this idea?
|
If it needs another macro anyway, I would rather integrate auto-dereference in the macro as well. I implemented Maybe it can be extended to call some particular field access methods generated by Clang.jl. In this way, the field access behavior are kept as-is, and end users get a handy macro. |
the main point here is that we can turn |
Yeah module MyPackage
module lib
include("../bindings.jl")
end
using CSyntax
x() = @c lib.foo().bar Presumably the field access methods are in |
|
|
Ah, nice 👍 And with luck that'll constant fold away. |
I ended up implementing Latest codegenusing CEnum: CEnum, @cenum
macro ptr(expr)
if !isa(expr, Expr) || expr.head != :.
error("Expression is not a property access")
end
quote
local penultimate_obj = $(esc(expr.args[1]))
(@__MODULE__).getptr(penultimate_obj, $(esc(expr.args[2])))
end
end
struct TypedefStruct
i::Cint
end
struct Other
i::Cint
end
function getptr(x::Ptr{Other}, f::Symbol)
f === :i && return Ptr{Cint}(x + 0)
error("Unrecognized field of type `Other`" * ": $(f)")
end
function Base.getproperty(x::Ptr{Other}, f::Symbol)
f === :i && return unsafe_load(getptr(x, f))
return getfield(x, f)
end
function Base.setproperty!(x::Ptr{Other}, f::Symbol, v)
unsafe_store!(getptr(x, f), v)
end
mutable struct WithFields
int_value::Cint
int_ptr::Ptr{Cint}
struct_value::Other
struct_ptr::Ptr{Other}
typedef_struct_value::TypedefStruct
array::NTuple{2, Cint}
end
function getptr(x::Ptr{WithFields}, f::Symbol)
f === :int_value && return Ptr{Cint}(x + 0)
f === :int_ptr && return Ptr{Ptr{Cint}}(x + 8)
f === :struct_value && return Ptr{Other}(x + 16)
f === :struct_ptr && return Ptr{Ptr{Other}}(x + 24)
f === :typedef_struct_value && return Ptr{TypedefStruct}(x + 32)
f === :array && return Ptr{NTuple{2, Cint}}(x + 36)
error("Unrecognized field of type `WithFields`" * ": $(f)")
end
function Base.getproperty(x::Ptr{WithFields}, f::Symbol)
f === :int_value && return unsafe_load(getptr(x, f))
f === :int_ptr && return unsafe_load(getptr(x, f))
f === :struct_value && return getptr(x, f)
f === :struct_ptr && return unsafe_load(getptr(x, f))
f === :typedef_struct_value && return getptr(x, f)
f === :array && return getptr(x, f)
return getfield(x, f)
end
function Base.setproperty!(x::Ptr{WithFields}, f::Symbol, v)
unsafe_store!(getptr(x, f), v)
end I want to play around with it a bit more in CImGui.jl before merging, but what do you think? |
Hmm, looks like nightly is broken:
|
IIUC, with the new flag |
Almost the same, the only difference is that with 129555d the logic for bitfields will be in standalone EDIT: added some tests for the default behaviour in 73ed7ff. |
I don't know how many people are using this package. But let’s take a poll and see if there are any unexpected problems. This PR provides an alternative method for field access. The default behavior is that accessing a field always returns a The opt-in behavior provided by this PR is that:
Please leave a 👍🏻 or 👎🏻 reaction, and any feedback is appreciated. |
Sounds good :) In the meantime I'll try fixing nightly. |
Alrighty, now we're back in the green 🟢 |
73ed7ff
to
3475e51
Compare
Old tarballs from previous Julia versions would otherwise screw up the use of `only()` to find the latest tarball.
This makes them easier to re-use elsewhere.
Copying the description from the code: > By default the getproperty!(x::Ptr, ::Symbol) methods created for wrapped > types will return pointers (Ptr{T}) to the struct fields. That behaviour is > useful for accessing nested struct fields but it does require explicitly > calling unsafe_load() every time. When enabled this option will automatically > call unsafe_load() for you *except on nested struct fields and arrays*, which > should make explicitly calling unsafe_load() unnecessary in most cases.
3475e51
to
d901cb7
Compare
Gentle bump, I rebased this and fixed a lingering bug in |
This reverts commit 9c77fd6. Now the tests fail for a different reason :')
CI should be fixed with 5f8a145 (reverts #504), unfortunately we're now hitting this: JuliaLang/julia#54664 |
I noticed some warnings on nightly about the field access exception type changing to Latest codegenusing CEnum: CEnum, @cenum
macro ptr(expr)
if !Meta.isexpr(expr, :.)
error("Expression is not a property access, cannot use @ptr on it.")
end
quote
local penultimate_obj = $(esc(expr.args[1]))
getptr(penultimate_obj, $(esc(expr.args[2])))
end
end
struct TypedefStruct
i::Cint
end
struct Other
i::Cint
end
function getptr(x::Ptr{Other}, f::Symbol)
f === :i && return Ptr{Cint}(x + 0)
@static if VERSION >= v"1.12.0-DEV"
throw(FieldError(Other, f))
else
error("Unrecognized field of type `Other`" * ": $(f)")
end
end
function Base.getproperty(x::Ptr{Other}, f::Symbol)
f === :i && return unsafe_load(getptr(x, f))
return getfield(x, f)
end
function Base.setproperty!(x::Ptr{Other}, f::Symbol, v)
unsafe_store!(getptr(x, f), v)
end
mutable struct WithFields
int_value::Cint
int_ptr::Ptr{Cint}
struct_value::Other
struct_ptr::Ptr{Other}
typedef_struct_value::TypedefStruct
array::NTuple{2, Cint}
end
function getptr(x::Ptr{WithFields}, f::Symbol)
f === :int_value && return Ptr{Cint}(x + 0)
f === :int_ptr && return Ptr{Ptr{Cint}}(x + 8)
f === :struct_value && return Ptr{Other}(x + 16)
f === :struct_ptr && return Ptr{Ptr{Other}}(x + 24)
f === :typedef_struct_value && return Ptr{TypedefStruct}(x + 32)
f === :array && return Ptr{NTuple{2, Cint}}(x + 36)
@static if VERSION >= v"1.12.0-DEV"
throw(FieldError(WithFields, f))
else
error("Unrecognized field of type `WithFields`" * ": $(f)")
end
end
function Base.getproperty(x::Ptr{WithFields}, f::Symbol)
f === :int_value && return unsafe_load(getptr(x, f))
f === :int_ptr && return unsafe_load(getptr(x, f))
f === :struct_value && return getptr(x, f)
f === :struct_ptr && return unsafe_load(getptr(x, f))
f === :typedef_struct_value && return getptr(x, f)
f === :array && return getptr(x, f)
return getfield(x, f)
end
function Base.setproperty!(x::Ptr{WithFields}, f::Symbol, v)
unsafe_store!(getptr(x, f), v)
end |
https://github.com/cjdoris/UnsafePointers.jl overloads |
maybe we should also adopt the way to overload |
Just to clarify, are you saying that |
This is what I'm currently thinking about:
|
I see 🤔 But yeah that sounds reasonable, and it's kinda nice to share some syntax with |
The following equality holds for all types
|
Copying the description from the code:
Here's the generated code for the
struct-properties.h
header in the tests:I've also tested it with CImGui.jl: JuliaImGui/CImGui.jl#131
I will admit the code is a bit hairy 🐙 Not sure if it's the cleanest implementation.
EDIT: marked as a draft because JET.jl found some issues with it.