Open JamesWrigley opened 2 weeks ago
The problem JET found was in how bitfields are handled, I believe that's fixed now. My approach was to:
Base.getproperty(x::T, f)
and Base.getproperty(x::Ptr{T}, f)
both call those functions, so they should both behave the same.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 unsafe_load()
'ing it automatically, e.g.:
# 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 _getptr(io, :ConfigFlags)
, but that's rather unsatisfying. So I had an evil thought :stuck_out_tongue: What if we add a third parameter to our Base.getproperty()
methods, like Base.getproperty(x::Ptr{T}, f::Symbol, as_ptr::Bool=false)
, and implement a @ptr
macro that turns @ptr foo.bar
into Base.getproperty(foo, :bar, true)
? Then folks could write e.g. @ptr io.ConfigFlags
whenever they want a pointer.
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 @test obj_ptr.struct_value.i == obj.struct_value.i
. The rule is that the dereferencing only occurs if the field type isn't a struct or array, so if there's a chain of nested structs Base.getproperty()
will return a pointer to each of them just like the current behaviour. But if there's a non-struct/array type at the end of the chain, like an int, then that field will be dereferenced and Base.getproperty()
on it will return a Cint
.
The rule is that the dereferencing only occurs if the field type isn't a struct or array,
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'm pretty sure this gonna confuse newbies, probably LLMs too...
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 ImVec2
field:
struct Foo
vec::ImVec2
end
Currently to access .x
and .y
they would need to do:
vec = unsafe_load(obj.vec)
vec.x, vec.y
Whereas with this PR they can simply do obj.vec.x
and obj.vec.y
. In any case, the feature is off by default so folks will have to explicitly opt in to use it. And I confess I don't care very much about LLM's :stuck_out_tongue:
see https://giordano.github.io/blog/2019-05-03-julia-get-pointer-value/ for a hack of dereference operator in Julia.
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 :eyes: 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?
The simple fix there would be to use
_getptr(io, :ConfigFlags)
, but that's rather unsatisfying. So I had an evil thought π What if we add a third parameter to ourBase.getproperty()
methods, likeBase.getproperty(x::Ptr{T}, f::Symbol, as_ptr::Bool=false)
, and implement a@ptr
macro that turns@ptr foo.bar
intoBase.getproperty(foo, :bar, true)
? Then folks could write e.g.@ptr io.ConfigFlags
whenever they want a pointer.
If it needs another macro anyway, I would rather integrate auto-dereference in the macro as well.
I implemented @c
macro in https://github.com/Gnimuc/CSyntax.jl to mimic the &
operator in C.
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 @ptr foo.bar
into any custom function call, there is not necessary to pirates Base.getproperty
.
Yeah @c
would actually be perfect :thinking: Unless I'm mistaken, I think we do need to use Base.getproperty()
though? The reason is that some code living outside of the bindings has no other way to know which getptr()
function to call. For example:
module MyPackage
module lib
include("../bindings.jl")
end
using CSyntax
x() = @c lib.foo().bar
Presumably the field access methods are in lib
, but @c
has no idea about that so it can't know it has to put in a call to lib.getptr
instead of getptr
. An alternative would be to define methods of functions defined in either Clang.jl or CSyntax.jl or CEnum.jl, but that would make either package a hard dependency.
Presumably the field access methods are in lib, but @c has no idea about that so it can't know it has to put in a call to lib.getptr instead of getptr. An alternative would be to define methods of functions defined in either Clang.jl or CSyntax.jl or CEnum.jl, but that would make either package a hard dependency.
parentmodule(typeof(Foo())).getptr
can do the trick.
julia> module Foo
struct Bar
x::Int
end
getx(x::Bar) = "x"
export Bar
end
Main.Foo
julia> using .Foo
julia> @generated function getxxx(x::T) where {T}
mod = parentmodule(T)
:($mod.getx(x))
end
getxxx (generic function with 1 method)
julia> macro c(x, ex...)
:(getxxx($(esc(x))))
end
@c (macro with 1 method)
julia> @c Bar(1)
"x"
Ah, nice :+1: And with luck that'll constant fold away.
I ended up implementing @ptr
in the module itself in 89be1a6 because then we can just use @__MODULE__
.
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 off
, we can get the same output as before, right?
Almost the same, the only difference is that with 129555d the logic for bitfields will be in standalone getbitfieldproperty()
/setbitfieldproperty!()
functions instead of being inline with Base.getproperty()
/Base.setproperty!()
. But the behaviour should be identical :crossed_fingers: I already added a test for that in the bitfield tests in 2e7e00d, but I'll add some more to make it doesn't regress.
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 Ptr
and one needs to call unsafe_load
manually to load/copy the value.
The opt-in behavior provided by this PR is that:
unsafe_load
to load/copy the value if the field type isn't a struct or array.@ptr
needs to be called manually.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 :green_circle:
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: https://github.com/Gnimuc/CImGui.jl/pull/131 I will admit the code is a bit hairy :octopus: Not sure if it's the cleanest implementation.
EDIT: marked as a draft because JET.jl found some issues with it.